move ocpapi to integration tests #736
Conversation
rayg1234
commented
Jun 20, 2024
•
rayg1234
commented
- Move OCP api integration tests to its own workflow, will only be triggered when changes are made to src/fairchem/demo/ directory, tests ran by making a small change to "src/fairchem/demo/ocpapi/README.md"
- re-enable some tests
.github/workflows/test.yml
Outdated
| pytest tests -vv --skip-ocpapi-integration --cov-report=xml --cov=fairchem -c ./packages/fairchem-core/pyproject.toml | ||
| pytest tests -vv --ignore=tests/demo/ocpapi/tests/integration/ --cov-report=xml --cov=fairchem -c ./packages/fairchem-core/pyproject.toml | ||
|
|
||
| - name: Integration tests |
There was a problem hiding this comment.
Will this run on every PR / commit on a PR? Or does it need to be targeted in some way?
These tests run the end to end demo so can get expensive if done on a CI workflow with lots of executions.
There was a problem hiding this comment.
so the by default this was running on every PR, if we want we can remove from CI all together, wdyt?
There was a problem hiding this comment.
Sorry, still catching up on how these tests are triggered. I assumed --skip-ocpapi-integration was skipping everything, but I think I understand now...
I would recommend adding --skip-ocpapi-integration to the command below. It was added in 2715160 in order to skip some especially expensive API calls (ones that are actually behind a pretty restrictive rate limiter as well). We don't want them to run on PRs, even if the errors are ignored.
In addition, I'm not sure the right test methods are decorated. These methods should have the decorator - they are very expensive and rate limited (I would not change them):
- https://github.com/FAIR-Chem/fairchem/blob/main/tests/demo/ocpapi/tests/integration/client/test_client.py#L169-L170
- https://github.com/FAIR-Chem/fairchem/blob/main/tests/demo/ocpapi/tests/integration/client/test_client.py#L261-L262
However, these methods are not very expensive. I'm not sure why they have the decorator (@misko ?). I would recommend removing it from:
- https://github.com/FAIR-Chem/fairchem/blob/main/tests/demo/ocpapi/tests/integration/client/test_client.py#L353
- https://github.com/FAIR-Chem/fairchem/blob/main/tests/demo/ocpapi/tests/integration/client/test_client.py#L366
- https://github.com/FAIR-Chem/fairchem/blob/main/tests/demo/ocpapi/tests/integration/client/test_client.py#L393
Finally, this test does hit an expensive / rate-limited API. I would recommend adding the decorator to it:
There was a problem hiding this comment.
One question on adding continue-on-error: true...
What happens if someone changes the client code in this repo in a way that actually break it? I understand the motivation here - that we don't want to block builds because a remote API is not behaving correctly. But this will hide breaking code changes as well, won't it?
There was a problem hiding this comment.
Moved integration tests out and re-enabled the above tests, removed the continue-on-error
Codecov ReportAll modified and coverable lines are covered by tests ✅ |
There was a problem hiding this comment.
Looks good. Just one question inline.
And would you mind adding @pytest.mark.ocpapi_integration_test to https://github.com/FAIR-Chem/fairchem/blob/main/tests/demo/ocpapi/tests/integration/workflows/test_adsorbates.py#L73. It should have been there before.
| pip install -e packages/fairchem-applications-cattsunami | ||
|
|
||
| - name: Integration tests | ||
| continue-on-error: true |
There was a problem hiding this comment.
You mentioned in your comment that you removed this. Did you want to keep it?
* move to integration tests * remove code cov for integration * move integration to separate workflow * add change to see if integration test triggers * remove skip markers * change to on pull request * add test back, remove continue-on-error
