Profile: formalize what constitutes a test profile #5392
Conversation
| def test_migrate_full(load_config_sample, monkeypatch): | ||
| """Test the full config migration.""" | ||
| config_initial = load_config_sample('input/0.json') | ||
| monkeypatch.setattr(uuid, 'uuid4', lambda: uuid.UUID(hex='0' * 32)) |
There was a problem hiding this comment.
Why is this necessary? I assume it targets these lines, but I'm not sure why it is needed.
There was a problem hiding this comment.
Because the migration will generate a random UUID, but we are checking against a static reference file that has 0 * 32 as UUID. If we don't force the same UUID, the comparison and so test will fail
There was a problem hiding this comment.
Mmm I see, thanks. I'm wondering if it makes sense to add a comment for this since we are not technically monkeypatching to isolate the module or to verify that the uuid lib is called, but just as a side-effect of how we do the tests, i.e. wanting to do direct reference file comparison (since if we were doing it manually migration by migration we would only to check that the 'PROFILE_UUID' keyword was added and not its content). Not sure how important it is to clarify this distinction though, so up to you, I'll approve either way.
|
Another general comment to think about: in this case you didn't actually change much in the tests so it was easy to review, but I did notice that if you had added any new tests to these or modified the behavior of old ones, it would have been much more difficult to review. This is because it is quite mentally costly to be changing from "just making sure the prior test and new tests do the same despite the (probably) big change in structure" vs "the new features are being correctly tested". I know this might be a bit idealistic on my part, but perhaps we could try taking your request of "try to update the old tests as you need to add more to that file" and make a small addendum of "try to do the update as a separate PR prior to the one that introduces the changes". |
|
Think that makes sense, at least for the reviewing process. We could definitely suggest that a first commit is added that modifies existing tests from old to new style, and then actual changes/additions come in a following commit. After review, we can then squash merge. Care needs to be taken if there are more than 1 commit that needs to be kept after merging, in which case we should manually interactively rebase to squash certain commits, before merging. |
| assert config_migrated == config_reference | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('initial, target', ( |
There was a problem hiding this comment.
You could auto-generate the migrations to test, from the MIGRATIONS list.
That would "guard" against people forgetting to update it when adding new migrations
(I do similar with the psql migrations)
There was a problem hiding this comment.
(made this comment earlier, but always forget that the github ios app adds comments as pending)
didn't actually notice that there were still some hold out |
sphuber
force-pushed
the
fix/5390/formalize-test-profile
branch
from
7c3f7a1 to
2ccb1fd
Compare
Up till now, a profile was defined as a test profile if its name started with `test_`. However, this is neither very robust nor very flexible as it restricts naming for profiles. Here we make it more explicit by adding the key `test_profile` to all profiles through a configuration migration. Profiles whose name starts with `test_` have `test_profile` set to `True`. Profiles that have `test_profile = True` are considered a test profile and so are allowed to be used in running the test suite. For safety reasons, the `Profile.is_test_profile` property returns `True` if and only if `test_profile` is set to `True`. If it is missing or any other value is set, it returns `False`. This prevents a profile accidentally being used for tests.
This new flag allows to designate a new profile that is created to be a test profile, which means it can be used to run the test suite.
sphuber
force-pushed
the
fix/5390/formalize-test-profile
branch
from
2ccb1fd to
c8104da
Compare
| profile_names = list(profiles.keys()) | ||
|
|
||
| # Iterate over the fixed list of the profile names, since we are mutating the profiles dictionary. | ||
| for profile_name in profile_names: |
There was a problem hiding this comment.
Nice! I see you are still pushing changes, so ping me when you want the final review.
There was a problem hiding this comment.
I am done. Everything is addressed and commits are rebased
|
@chrisjsewell you happy with this? |
yeh, just mentally preparing for more merge conflict in #5375 lol |
Fixes #5390
Up till now, a profile was defined as a test profile if its name started
with
test_. However, this is neither very robust nor very flexible asit restricts naming for profiles.
Here we make it more explicit by adding the key
test_profileto allprofiles through a configuration migration. Profiles whose name starts
with
test_havetest_profileset toTrue.Profiles that have
test_profile = Trueare considered a test profileand so are allowed to be used in running the test suite. For safety
reasons, the
Profile.is_test_profileproperty returnsTrueif andonly if
test_profileis set toTrue. If it is missing or any othervalue is set, it returns
False. This prevents a profile accidentallybeing used for tests.
CLI: add --test-profile flag to verdi setup/quicksetup
This new flag allows to designate a new profile that is created to be a
test profile, which means it can be used to run the test suite.