-
Notifications
You must be signed in to change notification settings - Fork 224
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
Consider 'AIIDA_TEST_PROFILE' in 'get_test_backend_name'. #3685
Consider 'AIIDA_TEST_PROFILE' in 'get_test_backend_name'. #3685
Conversation
Didn't end up doing anything with the |
aiida/manage/tests/__init__.py
Outdated
test_profile_name = get_test_profile_name() | ||
backend_env = os.environ.get('AIIDA_TEST_BACKEND', None) | ||
if test_profile_name is not None: | ||
backend_profile = configuration.load_config().get_profile(test_profile_name).database_backend |
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.
Is this the right way to access the profile information, or is there a better way?
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.
Almost, I think get_config()
(rather than load_config()
) would be the appropriate function here
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.
Fixed!
572f77b
to
66b995f
Compare
No idea why pre-commit is acting up -- seems similar to what was fixed in #3668. It seems the problem doesn't always happen, though. |
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 @greschd !
just a minor change request.
I guess this function is already tested enough via pytest, so we don't need to add an extra test...
aiida/manage/tests/__init__.py
Outdated
test_profile_name = get_test_profile_name() | ||
backend_env = os.environ.get('AIIDA_TEST_BACKEND', None) | ||
if test_profile_name is not None: | ||
backend_profile = configuration.load_config().get_profile(test_profile_name).database_backend |
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.
Almost, I think get_config()
(rather than load_config()
) would be the appropriate function here
Change the logic of 'get_test_backend_name' to check (if given) the backend set in the configuration for the profile specified in 'AIIDA_TEST_PROFILE'. If a backend is also specified in 'AIIDA_TEST_BACKEND', it checks that the two match, raising ValueError otherwise. If neither is specified, fall back to the default django backend.
66b995f
to
0f20908
Compare
Well, I could add a test where |
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 @greschd
Good to merge from my side - let's see whether pre-commit agrees ;-)
On a related note, should |
I don't see how it can - what if it is called before the test manager has taken over? |
I think you're right -- I was thinking of |
Change the logic of
get_test_backend_name
to check (if given) the backend configured for the profile specified inAIIDA_TEST_PROFILE
AIIDA_TEST_BACKEND
orAIIDA_TEST_PROFILE
), it will return the corresponding backendValueError
otherwiseAffects #3659.