-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Defend Workflows][E2E]Get file command from response console #156159
Merged
szwarckonrad
merged 73 commits into
elastic:main
from
szwarckonrad:endpoint-e2e-get-file
May 16, 2023
Merged
Changes from all commits
Commits
Show all changes
73 commits
Select commit
Hold shift + click to select a range
2f884d5
avatar aria label
szwarckonrad 704886c
isolate command e2e coverage
szwarckonrad feab480
Merge branch 'main' into endpoint-isolate-e2e-coverage
szwarckonrad 5db6e07
typings
szwarckonrad 51b63c3
Merge branch 'main' into endpoint-isolate-e2e-coverage
szwarckonrad 4de9906
typings
szwarckonrad 67eb92c
cleanup
szwarckonrad 12874a1
Merge branch 'main' into endpoint-isolate-e2e-coverage
szwarckonrad fcc702f
use custom document generator
szwarckonrad 25674ff
Merge branch 'main' into endpoint-isolate-e2e-coverage
szwarckonrad 7c9043f
Merge branch 'main' into endpoint-isolate-e2e-coverage
szwarckonrad c54509b
manualy refresh result list
szwarckonrad d2fbb5d
Merge branch 'main' into endpoint-isolate-e2e-coverage
szwarckonrad 9415935
remove artifacts after endpoints.cy.ts test
szwarckonrad ac9e110
Merge branch 'main' into endpoint-isolate-e2e-coverage
szwarckonrad 193968b
backport isolate e2e tests to multipass
szwarckonrad f84233b
Merge branch 'main' into endpoint-isolate-e2e-coverage
szwarckonrad b3a7460
cleanup
szwarckonrad 345c733
Merge branch 'endpoint-isolate-e2e-coverage' into endpoint-isolate-e2…
szwarckonrad b5b6941
tweaks
szwarckonrad 62fa777
Merge branch 'main' into endpoint-isolate-e2e-coverage
szwarckonrad 5157dda
Merge branch 'endpoint-isolate-e2e-coverage' into endpoint-isolate-e2…
szwarckonrad 7c13a7e
cleanup
szwarckonrad 890c52c
test isolate and processes commands
szwarckonrad 678aaa7
Merge branch 'main' into endpoint-e2e-coverage-multipass
szwarckonrad 7aea2b5
Merge branch 'main' into endpoint-isolate-e2e-coverage-multipass
szwarckonrad 2b07574
type returns of helper functions
szwarckonrad 7b8fe2a
Merge branch 'endpoint-isolate-e2e-coverage-multipass' into endpoint-…
szwarckonrad 12b16d2
tweaks
szwarckonrad aef3b55
Merge branch 'main' into endpoint-e2e-coverage-multipass
szwarckonrad c6c32bd
Merge branch 'main' into endpoint-e2e-response-console
szwarckonrad 68f07c9
test
szwarckonrad 7d03f5c
Merge branch 'main' into endpoint-e2e-coverage-multipass
szwarckonrad d588d6d
fix action
patrykkopycinski f5ce24d
divide endpoint list checking function
szwarckonrad 9a6c19d
Merge branch 'endpoint-e2e-coverage-multipass' into endpoint-e2e-resp…
szwarckonrad 71a138e
e2e coverage
szwarckonrad 44e0b5c
Merge branch 'main' into endpoint-e2e-response-console
szwarckonrad 28e459e
Merge branch 'main' into endpoint-e2e-coverage-multipass
szwarckonrad 02d2c1c
Merge branch 'main' into endpoint-e2e-response-console
szwarckonrad 2a2ed44
Merge branch 'main' into endpoint-e2e-coverage-multipass
szwarckonrad 983e5dc
Merge branch 'main' into endpoint-e2e-response-console
szwarckonrad 9f74e8f
Merge branch 'main' into endpoint-e2e-coverage-multipass
szwarckonrad dc61a76
cleanup
szwarckonrad c0f3f24
naming
szwarckonrad 7f0526f
naming
szwarckonrad d826943
explicit types
szwarckonrad bdce83f
Merge branch 'main' into endpoint-e2e-coverage-multipass
szwarckonrad 513c616
Merge branch 'endpoint-e2e-coverage-multipass' into endpoint-e2e-resp…
szwarckonrad 9fef80e
Merge remote-tracking branch 'origin/endpoint-e2e-response-console' i…
szwarckonrad 4090b64
Merge branch 'main' into endpoint-e2e-response-console
szwarckonrad 6e7e9b5
Merge branch 'main' into endpoint-e2e-response-console
szwarckonrad 15c4521
explicit types
szwarckonrad 6b4639f
check for spawned endpoint on the endpoint list as there might be dan…
szwarckonrad 4f8b8a1
move response actions out of emulator scope
szwarckonrad bedfb41
Merge branch 'main' into endpoint-e2e-response-console
szwarckonrad 40803c5
get file mocked data e2e
szwarckonrad 0809f48
Merge branch 'main' into endpoint-e2e-get-file
szwarckonrad 1aec910
Merge branch 'main' into endpoint-e2e-get-file
szwarckonrad cf905bc
Merge branch 'main' into endpoint-e2e-get-file
szwarckonrad 9eb8a10
Merge branch 'main' into endpoint-e2e-get-file
szwarckonrad 5da33e6
get file real endpoint e2e
szwarckonrad dd9e155
Merge remote-tracking branch 'origin/endpoint-e2e-get-file' into endp…
szwarckonrad cb4aa5f
Merge branch 'main' into endpoint-e2e-get-file
szwarckonrad b78b78a
tweaks
szwarckonrad e69493e
Merge remote-tracking branch 'origin/endpoint-e2e-get-file' into endp…
szwarckonrad f74767f
unskip response actions cy
szwarckonrad c6bf892
Merge remote-tracking branch 'upstream/main'
szwarckonrad 61a8a8f
Merge branch 'main' into endpoint-e2e-get-file
szwarckonrad 76ff802
Merge branch 'main' into endpoint-e2e-get-file
szwarckonrad fe4e325
Merge branch 'main' into endpoint-e2e-get-file
szwarckonrad 2a1a705
Merge branch 'main' into endpoint-e2e-get-file
szwarckonrad 058cb51
Merge branch 'main' into endpoint-e2e-get-file
szwarckonrad File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really feels overkill... All you are trying to test is that the endpoint can return a file based on a
get-file
action. Why not just retrieve a log file from the directory where endpoint is installed?I'm ok if you leave it, but it just feels like we're doing unnecessary work IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I was aiming at is an e2e coverage of user's journey, therefore I want to make sure user can download and extract the exact file they requested. Do you thinks that's going too far? If so then the rest of the changes doesn't make much sense indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can make sure that the user can retrieve a file from the host - goal here should be to ensure Endpoint is sending back the file to ensure we have round trip communication. to do that, you can just
get-file
and use the path to any file on the VM - example: one of the log files under the Agent's install directory.I was just really confused as to why you were
creatingFileOnEndpoint()
anduploadFileToEndpoint()
andreadZipFileContentOnEndpoint()
. Just feels all of these are not necessaryThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I went this way because I wanted to confirm that user is getting the file they requested and that the archive can be extracted with default password and I wouIdn't be able to do that with a random log file. In the process I found out that unzipping password protected zips in node can be tricky, its not platform agnostic when done locally so the most "stable" way is to do it on the host that runs ubuntu. It is cumbersome for sure but in the end its cheap time wise and covers what I believe is worth checking. It all comes down to the question whether checking the archive content adds value or not. Let me know what you think about it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its feels like you are testing the Endpoint and not really focusing on the test from Kibana. IMO, this is unnecessary and goes beyond what we should be focusing on. Go ahead an keep it if you feel strongly about this, but remember that we are not testing endpoint - we're testing that we can communicate with the endpoint for different actions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paul-tavares , isn't e2e test's purpose to validate if the user journey is successful? We don't specifically test if just one part of the functionality works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have different understanding of the level of testing we are validating with these real endpoint e2e tests. You seem to feel strongly that this is the right level so I'm going to retreat 😄 and let others provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since e2e tests are slow to write and run I would argue that we write tests that cover only the essential high-value flow for a user. In other words, we should only test high-value paths in the e2e tests that we can't test in unit/integration tests.
So in this example, it is enough to test that I can fetch an actual file from the endpoint and that I have a downloadable link, any file will do. I am not interested in verifying what the user does with it after it is fetched on the console. In terms of other console actions as well, it is enough to write a test that verifies that the action does what it is supposed to do. Send a request and check the response from the endpoint is what is expected.
I do understand the appeal of writing a bunch of tests to verify the entire click-by-click user interaction, but that would mean duplicating a bunch of tests that already cover that. Considering the effort required to write, maintain, and debug tests, I would argue against adding redundant tests for the whole user interaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey all, thanks for the discussion, great to see us thinking deeply about what and how we test.
Here's my thoughts:
Considering the above, we definitely achieve the initial goal of testing stack/Endpoint data flow ✅
I think validating that the user can download the file from Kibana is also something good to do here. Endpoint tests would not cover this particular scenario in the user journey as they are focused on Endpoint functionality itself and not what happens after file upload to ES is complete. So I think the extra coverage would be good here ✅
I think we should merge this test as is because it does automate a scenario that we would otherwise do manually. If I were to test this manually, I would certainly take the extra step to download and inspect the file. As we strive to eliminate manual testing, this seems like something good to do.
I acknowledge that this goes beyond our original thoughts around testing this area, but I see the value in these extra steps. I think it's good to always evolve the way we think about testing, especially early on with a new framework.