Fix #481: make tests of Pubchem more robust#1219
Conversation
In order to avoid failures in CI, this follows a [recommendation from Pavol Juhas on PR #1113]( #1113 (review)) about using mocking to avoid flaky test behavior in `pubchem_test.py`.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
mhucka
force-pushed
the
mh-revised-fix-481
branch
from
1b9ad4c to
70ddda8
Compare
mhucka
added
the
area/dependencies
mhucka
changed the title
|
/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 updates the PubChem interface to be compatible with a new version of the pubchempy library and improves the test suite by using mocking to avoid flaky tests. The changes are well-implemented. I've added a few suggestions to further improve the robustness of the API call retry mechanism and the structure of the test suite.
Pull request was converted to draft
mhucka
force-pushed
the
mh-revised-fix-481
branch
from
dda1837 to
1b8114b
Compare
There was a problem hiding this comment.
Code Review
This pull request updates the tests for the PubChem interface to use mocking, which resolves flakiness by avoiding calls to the live API in unit tests. It also introduces a new integration test for the live API, complete with a retry mechanism to improve robustness. The changes are well-structured and the new tests are thorough. I've identified some redundant try-except ImportError blocks in the new test methods. Since the test class is already skipped if pubchempy is not available, these blocks are unnecessary and can be removed or simplified for better code clarity.
Gemini Code Assist is right; they're not needed.
This is test code. This just doesn't seem worth doing more.
mhucka
changed the title
mhucka
changed the title
pavoljuhas
left a comment
pavoljuhas
left a comment
There was a problem hiding this comment.
For now let us avoid implementing retries for a test that is not selected.
If it is a problem later we can use the pytest-retry plugin for that one test instead.
Otherwise LGTM.
Per review comments, removing the retry code.
This is the result of running the pip-compile script, to account for the addition of `pytest-retry` to `dev_tools/requirements/deps/pytest.txt`.
Merge commits are not allowed on this repository
2 participants
This implements a recommendation from Pavol Juhas on PR #1113 about using mocking to avoid flaky test behavior in
pubchem_test.py.