🐛 FIX: Add `broker_*` parameters to default test config. #4831

greschd · 2021-03-24T09:19:47Z

In the TestProfileManager, set the broker_* parameters only if they are explicitly present in the profile_info.

Previously, these were set to None by default, which caused the config validation to fail because only string values are allowed.

I think the "root cause" why this wasn't caught is because there is validation in the Config constructor, but not the Profile constructor. This is the code constructing the profile:

aiida-core/aiida/manage/tests/__init__.py

Lines 333 to 336 in 75310e0

    
           config = configuration.get_config(create=True) 
        
           profile = Profile(profile_name, self.profile_dictionary) 
        
           config.add_profile(profile) 
        
           config.set_default_profile(profile_name).store()

Another option for fixing this issue would be explicitly setting the broker_* parameters in _DEFAULT_PROFILE_INFO:

aiida-core/aiida/manage/tests/__init__.py

Lines 28 to 42 in 75310e0

	_DEFAULT_PROFILE_INFO = {
	'name': 'test_profile',
	'email': '[email protected]',
	'first_name': 'AiiDA',
	'last_name': 'Plugintest',
	'institution': 'aiidateam',
	'database_engine': 'postgresql_psycopg2',
	'database_backend': 'django',
	'database_username': 'aiida',
	'database_password': 'aiida_pw',
	'database_name': 'aiida_db',
	'repo_dir': 'test_repo',
	'config_dir': '.aiida',
	'root_path': '',
	}

codecov · 2021-03-24T09:39:17Z

Codecov Report

Merging #4831 (b1c05b7) into develop (a8ee41d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #4831   +/-   ##
========================================
  Coverage    79.52%   79.52%           
========================================
  Files          519      519           
  Lines        37087    37087           
========================================
  Hits         29490    29490           
  Misses        7597     7597

Flag	Coverage Δ
django	`74.26% <ø> (ø)`
sqlalchemy	`73.17% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/manage/tests/__init__.py	`88.54% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8ee41d...b1c05b7. Read the comment docs.

ltalirz

thanks a lot for the fix @greschd

just one comment

ltalirz · 2021-03-24T10:11:56Z

aiida/manage/tests/__init__.py

            'repository_uri': f'file://{self.repo}',
        }
+        for key in ['broker_protocol', 'broker_username', 'broker_password', 'broker_host', 'broker_port']:


I guess currently these are the only ones regarded as optional, but one could also imagine e.g. the database_port having a default value in the future.

Why not use this loop for all keys of the dictionary?

Just to note,
for the broker defaults, they currently come from BROKER_DEFAULTS:

aiida-core/aiida/manage/external/rmq.py

Line 41 in 75310e0

BROKER_DEFAULTS = AttributeDict({

, and then the database defaults come from somewhere else.

But really I think all the defaults should derive from the schema defaults (in https://github.com/aiidateam/aiida-core/blob/develop/aiida/manage/configuration/schema/config-v5.schema.json), as happens for the options, i.e. the schema would be the "source of truth" for all defaults.

I just didn't have time to do this with the configuration refactor

Why not use this loop for all keys of the dictionary?

If null is invalid for all keys I agree that makes more sense.

Maybe getting the defaults for both the broker and the other options from the schema can be a separate PR?

ltalirz · 2021-03-24T10:14:31Z

I think the "root cause" why this wasn't caught is because there is validation in the Config constructor, but not the Profile constructor.

This is a bug, right?
At least at the point where the config with the new profiles is written to disk, we should validate.
It should not be possible for AiiDA to write an invalid config file.

This can be fixed in another PR if you prefer but I think this should be addressed.

chrisjsewell · 2021-03-24T10:34:52Z

This is a bug, right?
At least at the point where the config with the new profiles is written to disk, we should validate.

I'm sure we discussed this in #4712 and agreed not to validate (I can't find exactly where now though)

Note whenever you alter individual configuration options or when you use verdi setup/quicksetup these values are already validated then.
So in principle the config in memory will always be valid, because you validate on read then validate on modification. But in this test code you go through the "non-standard" route of generating the configuration

greschd · 2021-03-24T11:43:40Z

This can be fixed in another PR if you prefer but I think this should be addressed.

I'm not so up-to-date on the configuration code, so yeah it would probably be easier for someone else to address this in a separate PR.

ltalirz

thanks @greschd

greschd · 2021-03-24T13:25:34Z

Aah, seems some parts of the test suite require those keys to be there (even if they're None I guess).

Maybe the simplest way to fix this then is to add the broker_* parameters to _DEFAULT_PROFILE_INFO?

greschd · 2021-03-25T09:54:12Z

Alright, next try: Now I simply added the configuration values to _DEFAULT_PROFILE_INFO.

Initially I had them extracted programmatically from the config schema, but then I noticed not all keys are the same in profile_info as they are in the schema (e.g., the AIIDADB_* keys are renamed to database_*). So now I'm not sure if extracting them directly is reliable, or where this key mapping comes from.

In the `TestManager`, set the `broker_*` parameters by adding them to the `_DEFAULT_PROFILE_INFO`. The default values were copied from the config schema. They could be extracted directly, but since some keys (e.g. `AIIDADB_*`) are named differently in the `profile_info` compared to the config schema, this may be unreliable. Previously, these were set to `None` by default, which caused the config validation to fail because only string values are allowed.

chrisjsewell · 2021-03-25T10:04:06Z

cheers

So now I'm not sure if extracting them directly is reliable, or where this key mapping comes from.

The key mapping comes from:

aiida-core/aiida/manage/configuration/profile.py

Line 56 in a8ee41d

_map_config_to_internal = {

We also discussed removing this mapping in #4712 (but as with #4831 (comment), that PR was already big enough, so didn't want to add any more complexity)

greschd · 2021-03-25T10:08:32Z

Thanks @chrisjsewell - do you think it'd be better in this PR to hard-code things (and clean up together with further config refactoring), or go through the mapping and get these values from the schema?

I think, since there's a test checking that the test config validates, we should at least notice when things are broken now.

ltalirz

thanks @greschd - sorry for the back & forth

greschd · 2021-03-26T09:09:50Z

No problem, and thanks for the review @ltalirz!

) In the `TestManager`, set the `broker_*` parameters by adding them to the `_DEFAULT_PROFILE_INFO`. The default values were copied from the config schema. They could be extracted directly, but since some keys (e.g. `AIIDADB_*`) are named differently in the `profile_info` compared to the config schema, this may be unreliable. Previously, these were set to `None` by default, which caused the config validation to fail because only string values are allowed.

zhubonan · 2021-03-28T18:41:36Z

Thanks for the fix! The original bug appears to break plugin tests that involve running processes with the daemon.
Is there any plan to have a bug fix release? This would help with CI tests since we use the last released AiiDA version. Alternatively, we have to pin the tests to a patched AiiDA version.

chrisjsewell · 2021-03-28T18:44:24Z

yes there will be a release

espenfl · 2021-04-08T06:24:34Z

Thanks for fixing this folks. Any ETA on the release?

greschd · 2021-04-08T07:34:08Z

I think it's already released in 1.6.1.

chrisjsewell · 2021-04-08T07:51:41Z

Yep 👍

greschd requested review from ltalirz and chrisjsewell March 24, 2021 09:20

ltalirz reviewed Mar 24, 2021

View reviewed changes

greschd force-pushed the fix/test_fixtures_configuration branch from c82b591 to 4961d93 Compare March 24, 2021 11:48

ltalirz previously approved these changes Mar 24, 2021

View reviewed changes

greschd dismissed ltalirz’s stale review via 0ab25f1 March 25, 2021 09:51

greschd force-pushed the fix/test_fixtures_configuration branch from 4961d93 to 0ab25f1 Compare March 25, 2021 09:51

greschd force-pushed the fix/test_fixtures_configuration branch from 0ab25f1 to b1c05b7 Compare March 25, 2021 09:55

greschd changed the title ~~🐛 FIX: Remove null broker_* parameters from test config.~~ 🐛 FIX: Add broker_* parameters to default test config. Mar 25, 2021

greschd requested a review from ltalirz March 26, 2021 08:12

ltalirz approved these changes Mar 26, 2021

View reviewed changes

ltalirz merged commit 5b3e555 into aiidateam:develop Mar 26, 2021

greschd deleted the fix/test_fixtures_configuration branch March 26, 2021 09:09

zhubonan mentioned this pull request Mar 28, 2021

Sanitize nan and inf values before storing Dict nodes aiida-vasp/aiida-vasp#467

Merged

espenfl mentioned this pull request Apr 8, 2021

Fix a the fresh_aiida_env fixture aiida-vasp/aiida-vasp#472

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 FIX: Add `broker_*` parameters to default test config. #4831

🐛 FIX: Add `broker_*` parameters to default test config. #4831

greschd commented Mar 24, 2021

codecov bot commented Mar 24, 2021 •

edited

Loading

ltalirz left a comment

ltalirz Mar 24, 2021 •

edited

Loading

chrisjsewell Mar 24, 2021

greschd Mar 24, 2021

ltalirz commented Mar 24, 2021 •

edited

Loading

chrisjsewell commented Mar 24, 2021

greschd commented Mar 24, 2021

ltalirz left a comment

greschd commented Mar 24, 2021

greschd commented Mar 25, 2021

chrisjsewell commented Mar 25, 2021

greschd commented Mar 25, 2021

ltalirz left a comment

greschd commented Mar 26, 2021

zhubonan commented Mar 28, 2021

chrisjsewell commented Mar 28, 2021

espenfl commented Apr 8, 2021

greschd commented Apr 8, 2021 •

edited

Loading

chrisjsewell commented Apr 8, 2021

	config = configuration.get_config(create=True)
	profile = Profile(profile_name, self.profile_dictionary)
	config.add_profile(profile)
	config.set_default_profile(profile_name).store()

🐛 FIX: Add broker_* parameters to default test config. #4831

🐛 FIX: Add broker_* parameters to default test config. #4831

Conversation

greschd commented Mar 24, 2021

codecov bot commented Mar 24, 2021 • edited Loading

Codecov Report

ltalirz left a comment

Choose a reason for hiding this comment

ltalirz Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

chrisjsewell Mar 24, 2021

Choose a reason for hiding this comment

greschd Mar 24, 2021

Choose a reason for hiding this comment

ltalirz commented Mar 24, 2021 • edited Loading

chrisjsewell commented Mar 24, 2021

greschd commented Mar 24, 2021

ltalirz left a comment

Choose a reason for hiding this comment

greschd commented Mar 24, 2021

greschd commented Mar 25, 2021

chrisjsewell commented Mar 25, 2021

greschd commented Mar 25, 2021

ltalirz left a comment

Choose a reason for hiding this comment

greschd commented Mar 26, 2021

zhubonan commented Mar 28, 2021

chrisjsewell commented Mar 28, 2021

espenfl commented Apr 8, 2021

greschd commented Apr 8, 2021 • edited Loading

chrisjsewell commented Apr 8, 2021

🐛 FIX: Add `broker_*` parameters to default test config. #4831

🐛 FIX: Add `broker_*` parameters to default test config. #4831

codecov bot commented Mar 24, 2021 •

edited

Loading

ltalirz Mar 24, 2021 •

edited

Loading

ltalirz commented Mar 24, 2021 •

edited

Loading

greschd commented Apr 8, 2021 •

edited

Loading