Unit test fixing - remove network requirements #217
Conversation
So we can run them in isolation while fixing the tests
… moving things like citation count that cause spurious failures
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces changes to test configurations and adds mocking to remove network dependencies in several tests. The changes involve modifying pyproject.toml and pytest.ini to include new test markers, and updating test files to use mocking for external API calls. This aims to improve the reliability and speed of unit tests by eliminating their reliance on live network connections.
|
|
||
|
|
||
| @pytest.mark.ols |
There was a problem hiding this comment.
This test is marked as @pytest.mark.ols but it is missing the mock_ols_api fixture as an argument. This means the test will still attempt to make network calls, defeating the purpose of the mocking fixture. This needs to be corrected.
| @pytest.mark.ols | |
| def test_can_fetch_a_go_term(): |
| "ci: Tests that should run in continuous integration", | ||
| "slow: Tests that take a long time", | ||
| "db: Test that require access to our database", | ||
| "network: Tests that require network", |
There was a problem hiding this comment.
Adding network marker to pyproject.toml to mark tests that require network access. This is good for categorizing tests, but it's important to ensure that all tests that actually use the network are marked, and that tests are not unnecessarily marked as requiring network access.
| } | ||
|
|
||
| @pytest.mark.ensembl | ||
| @pytest.mark.network |
There was a problem hiding this comment.
The @pytest.mark.network marker is no longer needed here, as the test has been refactored to use mocking and no longer depends on a network connection. Removing this marker will accurately reflect the test's independence from network resources.
| @pytest.mark.network | |
| def test_builds_with_known_bands(): |
|
I'm not sure I like this overall. The upside to these things failing is sometimes the APIs and the like change without notice. This does catch them. There are ways to just log which tests are flakly and I'd rather start with that to be honest. |
|
100%, we need tests to catch changes in the APIs. My plan was to separate them out. So we have our assumptions about the data fromat in these tests, which run fast and can be put into CI on github actions or something. Then we have separate tests that run less frequently (a month before release?) to check the API matches our assumptions. My problem is that these tests fail for completely irrelevant things like the citation count changing, or the inclusion of an extra full stop in an ontology term definition. If we keep them on the live API, they will always fail, and my worry iss we will assume it is because of the dumb reason when a bigger API change happened. I would propose having bigger integration tests that use the live API and don't check the fine details like these do, or have specific API data contract testing checks that compare the mocked structure to the real one. This was just something I decided should be fixed one evening, so very happy to have any input! |
Our unit tests for the pipeline don't run very often, and when they do, they fail on things that are irrelevant, partly because they use live APIs and data changes.
This PR aims to remove a lot of the network access by mocking the API calls. This has been done by recording the output of API calls and using that to build mock data structures, applied by
unittest.mockpatches so the tests don't use the real API.This is a work in progress, and will touch many files, so I intend to do incremental PRs to keep reviews manageable.
This batch removes network calls for the OLS, tmRNA, ensembl karyotype and core PDBe tests.