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
|
|
||
|
|
||
| def mock_get_compounds(name, searchtype, record_type='2d'): | ||
| if name == 'water' and record_type == '3d': |
|
|
||
| @using_pubchempy | ||
| class OpenFermionPubChemTest(unittest.TestCase): | ||
| def _get_geometry_with_retries(self, name): |
There was a problem hiding this comment.
This is only used in test_geometry_from_pubchem_live_api which is deselected by default. I suggest to delete it and call geometry_from_pubchem directly.
If the live service proves to be a problem for the CI test, I suggest to add the pytest-retry plugin and decorate test_geometry_from_pubchem_live_api with its flaky mark.
|
|
||
| @patch('time.sleep', return_value=None) | ||
| @patch('pubchempy.get_compounds') | ||
| def test_geometry_from_pubchem_retry_success(self, mock_get_compounds, mock_sleep): |
There was a problem hiding this comment.
This is testing a test helper _get_geometry_with_retries, which is itself unnecessary per comment above. Tests should be only covering the public API (possibly protected APIs in exceptional cases), but testing a test code should be avoided. Please delete.
|
|
||
| @patch('time.sleep', return_value=None) | ||
| @patch('pubchempy.get_compounds') | ||
| def test_geometry_from_pubchem_retry_failure(self, mock_get_compounds, mock_sleep): |
There was a problem hiding this comment.
Same as above, please delete.
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.
2 participants
This implements a recommendation from Pavol Juhas on PR #1113 about using mocking to avoid flaky test behavior in
pubchem_test.py.