Move functions from test_applications_from_config.py to simtools.testing #1238
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Copilot reviewed 6 out of 12 changed files in this pull request and generated 1 suggestion.
Files not reviewed (6)
- tests/unit_tests/testing/test_compare_output.py: Evaluated as low risk
- .github/workflows/CI-schema-validation.yml: Evaluated as low risk
- tests/integration_tests/test_applications_from_config.py: Evaluated as low risk
- pyproject.toml: Evaluated as low risk
- docs/source/api-reference/testing.md: Evaluated as low risk
- docs/changes/1238.maintenance.md: Evaluated as low risk
Comments skipped due to low confidence (2)
simtools/testing/helpers.py:12
- [nitpick] The error message is too verbose. Consider making it more concise, e.g., 'Skipping due to old testeff version. Update sim_telarray tarball.'
pytest.skip("Any applications calling the old version of testeff are skipped due to a limitation of the old testeff not allowing to specify the include directory. Please update your sim_telarray tarball.")
simtools/testing/helpers.py:21
- [nitpick] The error message is too verbose. Consider making it more concise, e.g., 'Skipping test due to unimplemented fake SST mirrors (Customize testeff command for SST #1155).'
pytest.skip("The test simtools-validate-camera-efficiency_SSTS is skipped since the fake SST mirrors are not yet implemented (#1155)")
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
||
| def _prepare_configuration(config, output_path, model_version=None): | ||
| """ | ||
| Prepare test configuration. |
There was a problem hiding this comment.
Naming and desciption is very similar to above function configuration. Suggest to rename it and also to clarify what configuration string means in the docstring.
There was a problem hiding this comment.
Renamed it to _prepare_test_options. Improved also the docstring.
simtools/testing/configuration.py
Outdated
| ] | ||
| ) | ||
|
|
||
| if get_test_names: |
There was a problem hiding this comment.
Suggest to always return both, to avoid calling it twice in test_application_from_config.
There was a problem hiding this comment.
Good point, I've changed the logic accordingly.
There was a problem hiding this comment.
@GernotMaier thanks for the PR! The changes look good overall, I have added few minor comments.
Analysis Details0 Issues
Coverage and Duplications
Project ID: gammasim_simtools_AY_ssha9WiFxsX-2oy_w |
|
@tobiaskleiner - I think I've addressed your suggestions. Let me know if there is anything else I can do. Thanks for the review! |
The integration test application
test_applications_from_config.pycontains rather complicated functions to prepare, run, and validate the integration tests.This PR moves:
simtools.testingareatesting.configurationandtesting.helperstesting.compare_outputtotesting.validate_outputSide note: needed to move the pytest package from the test section to the general in pyproject toml, as some of modules in simtools.testing are importing pytest.
I suggest not to do a line-by-line code review, but review the general refactoring and naming. No new functionality has been added, but code moved around. Also not sure if we need a line-by-line review of units tests.
Closes #1174