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
Update test coordinates on master builds #1246
Conversation
@@ -0,0 +1,2 @@ | |||
#!/bin/bash | |||
python integration-test.py -printcoords | python scripts/update-integration-test-coordinates.py |
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 integration-test.py
in the current working directory? Should we use the script location to make sure that it finds the right file?
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.
It will be based on how it's run from circleci. The root of the repo is the assumed cwd.
What do you suggest instead? In the setup and run script I see this:
basedir="$(dirname ${BASH_SOURCE[0]})/.."
Should we do the same thing here?
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 so. That way it doesn't come as a surprise if someone tries to run it out of the scripts/
dir, or we end up in a $PWD
that we didn't expect.
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.
Btw, I don't mean to suggest that the way it is at the moment is wrong. When I read it, I thought "is integration-test.py
in the scripts/
directory?" and it took me a moment to figure out what was going on. I think it's better to avoid confusion (perhaps it's only my confusion 😖 ) by being explicit about the locations of files where we can.
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.
Updated in 0fffea0.
This should update the list of test coordinates in s3 on successful master builds.