-
Notifications
You must be signed in to change notification settings - Fork 8
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 cli options for config file management #33
Conversation
Add cli option: --require-aiida-testing-config: raise exception, if testing configuration is not found or if required code labels are missing. --write-aiida-testing-config: write template of config file to disk that contains labels of required codes.
even the git checkout is now failing...
Just to point out that some strange stuff was going on with github actions (see e.g. the ci results of "add test for regeneration of test data"). Also, on my computer, the 4 tests that involve running the CalcJob fail with error messages
(i.e. check_diff is empty for those). Haven't figured out why yet. |
Make sure path to data directory exists and is absolute. This fixes my local tests.
This seems to have been fixed by requiring the data directory to be an absolute path. |
Paths may contain spaces etc.
+ remove leftover 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.
Very nice, thanks! I've added a few minor comments.
For regenerating the data, I think the "export cache" feature will need a similar flag, but they should probably be independent: Maybe we should rename --regenerate-test-data
to something like --regenerate-mock-code-data
, to distinguish them?
aiida_testing/mock_code/_fixtures.py
Outdated
): | ||
ignore_files: ty.Iterable[str] = ('_aiidasubmit.sh'), | ||
executable_name: str = '', | ||
config: dict = testing_config, |
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.
Are config
, config_action
and regenerate_test_data
here passed just for being able to test the fixture internally, or do we expect users to set them?
I'm not really sure if there's an established convention for that, but maybe we could indicate this by a leading underscore in the kwarg names (_config
, _config_action
, _regenerate_test_data
)?
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.
done!
aiida_testing/mock_code/_fixtures.py
Outdated
config_action : | ||
If 'require', raise ValueError if config dictionary does not specify path of executable. | ||
If 'generate', add new key (label) to config dictionary. | ||
generate_config : |
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 generate_config
parameter doesn't exist, and regenerate_test_data
is missing in the docstring.
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.
thanks, fixed!
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.
Thanks! Do you want to create a new issue for the "add docs" part?
happy to add them in here, now that we've agreed on the names |
fixes #30
fixes #8
fixes #13
Add cli option:
--testing-config-action: Read config file if present ('read'), require config file ('require') or generate new config file ('generate').
--regenerate-test-data: Regenerate test data
A few things left to do: