Skip to content
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

Merged

Conversation

greschd
Copy link
Member

@greschd greschd commented Dec 18, 2019

Change the logic of get_test_backend_name to check (if given) the backend configured for the profile specified in AIIDA_TEST_PROFILE

  • If only one of the two is set (AIIDA_TEST_BACKEND or AIIDA_TEST_PROFILE), it will return the corresponding backend
  • If both are set, it checks that they match, raising ValueError otherwise
  • If neither is specified, fall back to the default django backend.

Affects #3659.

@greschd greschd requested a review from ltalirz December 18, 2019 12:55
@greschd
Copy link
Member Author

greschd commented Dec 18, 2019

Didn't end up doing anything with the pytest hooks - I think making get_test_backend_name understand that there can also be a AIIDA_TEST_PROFILE is a cleaner solution.

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
Copy link
Member Author

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?

Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@greschd greschd force-pushed the fix_backend_with_test_profile_envvar branch from 572f77b to 66b995f Compare December 18, 2019 13:01
@greschd
Copy link
Member Author

greschd commented Dec 18, 2019

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.

Copy link
Member

@ltalirz ltalirz left a 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...

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
Copy link
Member

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.
@greschd greschd force-pushed the fix_backend_with_test_profile_envvar branch from 66b995f to 0f20908 Compare December 18, 2019 14:01
@greschd
Copy link
Member Author

greschd commented Dec 18, 2019

Well, I could add a test where os.environ is changed manually.. but that would end up with almost the same logic as the function itself for figuring out what the result should be, so I'm not sure if it would be useful at all.

@ltalirz ltalirz self-requested a review December 18, 2019 14:09
Copy link
Member

@ltalirz ltalirz left a 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 ;-)

@greschd
Copy link
Member Author

greschd commented Dec 18, 2019

On a related note, should get_test_profile_name return the name of the automatically-created profile when AIIDA_TEST_PROFILE is not set?

@ltalirz
Copy link
Member

ltalirz commented Dec 18, 2019

On a related note, should get_test_profile_name return the name of the automatically-created profile when AIIDA_TEST_PROFILE is not set?

I don't see how it can - what if it is called before the test manager has taken over?
And even if it is called after the test manager has taken over, should it just return the default profile?
Maybe I'm missing what you have in mind?

@greschd
Copy link
Member Author

greschd commented Dec 18, 2019

I think you're right -- I was thinking of aiida-pytest, where I was only ever creating one automatic profile.

@ltalirz ltalirz merged commit 3402c3e into aiidateam:develop Dec 18, 2019
@greschd greschd deleted the fix_backend_with_test_profile_envvar branch December 18, 2019 15:06
@ltalirz ltalirz mentioned this pull request Dec 19, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants