-
Notifications
You must be signed in to change notification settings - Fork 40
TST: Use tmp_path fixture for temporary files in unit tests #489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Lucia-Fonseca
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code fixes the problem.
However, I wonder whether unittest.mock is more suitable for mocking files, so we don't need to create temporary files or have empty yml files(?)
| # Process empty config file | ||
| filename = get_pkg_data_filename('data/empty_config.yml') | ||
| assert skypy.main([filename, 'empty.fits']) == 0 | ||
| config_filename = get_pkg_data_filename('data/empty_config.yml') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't we use a temporary YAML file here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a (non-temporary) input file that is part of the repository
| pipeline = Pipeline(config) | ||
| pipeline.execute() | ||
| output_filename = 'output.fits' | ||
| output_filename = str(tmp_path / 'output.fits') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the str command? It is not used in the tutorial
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"tmp_path is a pathlib.Path object" but our script takes path names as strings
This PR is not mocking any files. It is implementing a more robust method (a temporary directory) to automatically handle the deletion of files created by the skypy script during testing. We don't want to mock those files; the fundamental purpose of these unit tests is to check that those files are created correctly. Mocking config files might be possible but is unrelated to the issue this PR is addressing and I don't see an obvious benefit? |
* Update name of default branch to main (#434) * update mailmap (#432) * Write all tables to a single FITS/HDF5 file (#425) * ADR 3: Position sampling and patches of the sky (#422) * BUG: Raise ImportError if optional dependency speclite is not installed (#437) * MAINT: Set NumPy latest supported version to 1.20 #440 * Update status badges (#441) * MAINT: Update Lucia affiliation (#451) * MAINT: add SIT's information (#450) * DOC: Fix contributor guidelines link (#449) Co-authored-by: Richard R <58728519+rrjbca@users.noreply.github.com> Co-authored-by: Nicolas Tessore <n.tessore@ucl.ac.uk> * ENH: Logging for skypy command line script and Pipeline class (#453) * DOC: Describe speclite filters in documentation (#457) * ENH: Config syntax for importing objects (#463) * DOC: List of Features (#456) * DOC: How to construct config files (#454) * DOC: Remove docstring examples (#429) * MAINT: Update Zenodo entry for RPR (#468) * DOC: Readme updates (#460) * DOC: Expanded landing page documentation (#228) * DOC: Inverse transform sampling accuracy warning (#472) * MAINT: Set astropy latest supported version to 4.2 (#483) * DOC: zenodo json members update (#481) * DOC: Ryden04 ellipticity doc missing section (#477) * MAINT: Update numpy and scipy latest supported versions (#488) * BUG: Change invalid ecsv datatype from int to uint16 (#485) * DEV: setuptools==58.0.0 (#493) Co-authored-by: Nicolas Tessore <n.tessore@ucl.ac.uk> * Add compatibility workflow badge (#487) * DEV: Enable pip to install pre-releases in the tox dev environments (#491) * TST: Use tmp_path fixture for temporary files in unit tests (#489) * BUG: Move handling of context arguments after handling of .depends keyword (#465) * BLD: Set astropy latest supported version to 4.3 and speclite minversion to 0.14 (#486) * REV: restore setuptools to latest version on readthedocs (#494) * DEV: pyparsing<3.0.0 (#500) * Check new astropy file overwrite error message in logging test (#498) * REV: restore pyparsing to latest version for doc builds (#501) * DOC: Update citation file with JOSS paper reference (#496) * BLD: Set astropy latest supported version to 5.0 (#504) * BLD: Set python latest supported version to 3.10 (#505) * BLD: Set numpy latest supported version to 1.22 (#506) * BLD: Set python oldest supported version to 3.7 (#507) * DOC: Fix code of conduct link (#508) * Changed y-label in luminosity function example. (#512) * BLD: Set scipy latest supported version to 1.8 (#510) * ENH: Rykoff model of the magnitude uncertainty (#526) * TST: assert photometric error is numerically close to the analytic value (#545) * TST: Drop deprecated astropy.tests.helper.raises (#546) * ENH: compute kcorrect remaining stellar mass (#476) * compute kcorrect remaining stellar mass * added test for stellar mass remain Co-authored-by: Ian Harrison <itrharrison@gmail.com> Co-authored-by: Lucia F. de la Bella <55983939+Lucia-Fonseca@users.noreply.github.com> * ENH: Logistic completeness function (#521) * BLD: Set astropy latest supported version to 5.1 (#547) * BUG: `schechter_smf` callable input and docs (#525) * DOC: Typo in Rykoff error (#550) * add Fox's details (#551) Co-authored-by: Richard R <58728519+rrjbca@users.noreply.github.com> * BLD: Set numpy latest supported version to 1.23 (#552) * codestyle fixes * add six requirement for colossus * tried to fix docs builds * update passenv * rtd configuration Co-authored-by: Richard R <58728519+rrjbca@users.noreply.github.com> Co-authored-by: Nicolas Tessore <n.tessore@ucl.ac.uk> Co-authored-by: Lucia F. de la Bella <55983939+Lucia-Fonseca@users.noreply.github.com> Co-authored-by: Sut-Ieng Tam <30295725+sutieng@users.noreply.github.com> Co-authored-by: philipp128 <48715661+philipp128@users.noreply.github.com> Co-authored-by: Fox Davidson <93545862+Fox-Davidson@users.noreply.github.com>
Description
Use pytest
tmp_pathfixture in unit tests. Creates a temporary directory where temporary files can be written and handles cleanup automatically. Resolves #466Checklist