Fix broken tests #156
Merged
Fix broken tests #156
Conversation
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
Faker was added on gc in comic/grand-challenge.org#3093, but it was not added to poetry and it is not available in the production container.
Docker is not available in the production container, so we use a local version of evaluation fixtures that loads the container from the local filesystem instead of building it with docker. Also we add another algorithm with a file input for testing.
This functionality is removed in #3156 on grand challenge so the tests are removed here.
This reverts commit 0cda3ae.
|
It seems there is one flaky test remaining. See this test run: https://github.com/DIAGNijmegen/rse-gcapi/actions/runs/10113674491/attempts/1 I reran the workflow without changing anything and then it passed. |
|
I think it would be best if we completely decouple the two repositories. These problems pop up often as the Grand Challenge codebase changes a lot, and we do not have automated tests for drifts. I know the tests well so am happy to do that today. |
jmsmkn
reviewed
Aug 1, 2024
This updates the supported versions of python, brings the fixture creation under version control here, removes the static container and disables a couple of tests as they were broken with the fixtures, will re-enable those later.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Warning: This PR is based on the remove_retina branch
I started by seeing when the tests did pass and if that is still the case currently. Looking at the latest merge it was around june 20 2023 so I started with gc around there (
6ef429ae7691afcadfbfae6e1da834ce0e3ba76b), this passes. Then I iterated over some releases to see when the first errors pop up. This happened atcc514a444386d1787a7b4ef5d1395c22e33df07d.Reason: Faker is added here but not added in poetry. It is available in the test container because it is a dependency of factory-boy, however it is not available in the production container, which is the container used by gcapi to run a local gc in the tests. This needs to be fixed by either adding it in the production container or removing the usage in the fixtures. For now, the easy fix was to "hack it in":
An algorithm with file as input was used in the tests but it is removed here: comic/grand-challenge.org#3140. I've added it back in a local version of algorithm_evaluation_fixtures.py.
Also in that PR, the demo algorithm used in the fixtures is built using docker. Docker is not available in the production container, so in the local version of evaluation fixtures we load the algorithm from the local filesystem.
Functionality for updating CIV interface by uploading existing file is removed here: comic/grand-challenge.org#3156. I've removed the tests and some documentation for this functionality.
At this point the codebase was at the removal of the retina endpoints: comic/grand-challenge.org#3192. This obviously introduced errors because of the missing endpoints. After merging #154, those errors dissapear, except for 2 new errors. Those are fixed by switching a token and removing string interpolation issue.
Then, after updating to the most recent version of gc, there were 2 issues remaining:
After all this the tests pass again 🎉
Some of the fixes here are hacky, we need to find a proper solution for them. Also we need to find a proper solution for keeping gcapi up to date with gc because otherwise changes will creep in that we miss here. Then we have broken version of gcapi and run into failing tests when we create a PR here. Maybe the tests here should be run periodically (weekly/bimonthly?) so that we find new errors sooner.