-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add default output directory #462
base: main
Are you sure you want to change the base?
Conversation
@@ -26,10 +26,10 @@ def test_help(): | |||
|
|||
def test_eos(): |
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 not just use pytest
's tmp_path
factory fixture?
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.
We do generally, but that doesn't test that the default file_prefix
creates the files we expect. (Unless we did something like change directories into a temporary directory, and ran everything from there)
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.
Could monkeypatch
to check the directory it would create and then over-write the return to tmp_path
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.
We probably could, but the exact way the filenames are built varies a little, so I'm not sure how easy it would be in a completely general way, and I think we might risk missing unusual interactions.
Arguably the current form is more of an integration test, since we test the file builder separately anyway, which I think is practically what we'd be checking again if we don't write to the actual default location.
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.
Yeah. That was one of my other thoughts. Leaving behind miscellaneous files (even empty folders) in the test suite just feels unpleasant. I think at the very least we want to delete them before attempting the test.
Co-authored-by: Jacob Wilkins <46597752+oerc0122@users.noreply.github.com>
773e6f2
to
bc0397e
Compare
Changes default
file_prefix
to include./janus_results/
(originallyresults
, but this clashes with mace training outputs, meaning that output files will normally all be contained within this directory, as discussed in #359.I initially thought we may want a dedicated option for the file directory, but since
file_prefix
directories are created anyway, something like--file-prefix ./alt_dir/prefix
, or--file-prefix ./just-prefix
(to avoid creating a new directory), which both already work, is just as effective.One slight question mark on the dev side of things is currently this will create the
janus_results
directory if it doesn't exist, including when running tests, but I only clean up the files, not the directory itself, so running the tests may leave behind a new empty directory. I thought this was probably preferable to a lot of extra logic figuring out if it starts empty and so is safe to delete/potential test failures if it's not.