-
Notifications
You must be signed in to change notification settings - Fork 594
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
copy ci test output files to artifacts dir on script exit #1177
copy ci test output files to artifacts dir on script exit #1177
Conversation
test-prow-e2e.sh
Outdated
./test-gui.sh e2e | ||
|
||
cp -rv ./frontend/gui_test_screenshots "${ARTIFACT_DIR}/gui_test_screenshots" | ||
. ./test-gui.sh e2e |
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.
Is there a reason this needs to be . ./test-gui.sh e2e
instead of just ./test-guit.sh e2e
?
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 will run the script and not exit the entire script if the test-gui.sh sub script returns an error. The trap should handle the error case I believe....I can remove this in the test pr and check the outcome.
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'd think the trap would still work if we run it directly. Can we test?
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.
Yeah the test is running.
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.
/lgtm
👍 thanks @jcaianirh
/retest |
/test console-e2e |
use trap to copy test files to artifacts dir on exit of script for e2e tests