Skip to content
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

Fixing gold standard script to work with circle ci #133

Merged
merged 6 commits into from May 7, 2020

Conversation

chummels
Copy link
Collaborator

@chummels chummels commented May 2, 2020

This should fix issue #132 so the gold standard script now work with circle_ci instead of travis.

@chummels chummels requested a review from brittonsmith May 2, 2020 21:48
@chummels
Copy link
Collaborator Author

chummels commented May 2, 2020

Actually, no, this doesn't address everything. This merely fixes the breakage of the script in accessing the travis yml file. But all of the other text in the script that helped you actually build the gold standard was removed, presumably during the switch to circle ci. I'm not sure why. Was there something wrong with the build order that was given to the user described in the docs: https://trident.readthedocs.io/en/latest/testing.html#generating-gold-standard-answer-test-results-for-comparison ? It's useful to be explicit with the user in regenerating thegold standards, and I personally find it helpful when I haven't built the gold standards in a long time to remember the steps.

@coveralls
Copy link

coveralls commented May 2, 2020

Pull Request Test Coverage Report for Build 741

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 75.209%

Totals Coverage Status
Change from base Build 674: 0.0%
Covered Lines: 1796
Relevant Lines: 2388

💛 - Coveralls

@brittonsmith
Copy link
Collaborator

I'm not sure what other text you're referring to. Between this script and the docs you link to, I don't see anything else that one needs to know to run testing. The build script is written to most efficiently use the testing workflow set up by the service, nothing more. It's not meant to be used as documentation. I missed this because it's not tested functionality. I left a comment on how it could be.

@chummels
Copy link
Collaborator Author

chummels commented May 6, 2020

@brittonsmith You're right. I had mistakenly thought that the text that was in the docs about how to generate the gold standards was also meant to be printed to STDOUT as part of this script. But I looked back at past versions and that was never the case. Sorry about that.

@chummels
Copy link
Collaborator Author

chummels commented May 6, 2020

I have now updated the script to include a test_gold.py file that will ensure that this functionality is tested in the future. Although I don't foresee us changing the testing infrastructure again, but I guess more tests are always better.

Copy link
Collaborator

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent. This looks great. Definitely an unlikely just in case test, but I'm happy it's there. I'll merge this. Thanks for doing it.

@brittonsmith brittonsmith merged commit eaaae0c into trident-project:master May 7, 2020
@chummels chummels deleted the fix_gold branch May 7, 2020 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants