Revision 7b139906ac46: Add NOT NULL constraints to certain Variable (meta_vars) columns #111

helfy18 · 2022-01-19T18:01:28Z

Resolves #29

Add not null constraints to meta_vars.

Add not null constraints to three variables in the meta_vars table modify the subset data so that the constraint is not violated

rod-glover

Looking good so far, Johnathan.

Minor point: Suggest you run Black on
pycds/alembic/versions/7b139906ac46_add_not_null_constraint_on_meta_vars.py
tests/climate_baseline_helpers/conftest.py
to clean up their formatting.

I think it is worth creating a test to test the data migration part of the migration (hmm, a lot of repetitions there). The test have to do something very like this:

establish a database at the previous revision
populate the meta_vars table with some rows with null values and some without
run the upgrade to your revision
check that meta_vars has been updated as expected - the nulls have been replaced

See if you can figure out how to do this by looking at other tests. This particular test will be best placed alongside the smoke tests for this migration, not under behavioural.

rod-glover

Good work, @helfy18 ! You've done a great job of checking the schema migration. There's really only one thing left -- checks for the data migrations. Comment below outlines how.

pycds/alembic/versions/7b139906ac46_add_not_null_constraint_on_meta_vars.py

rod-glover · 2022-01-26T00:18:53Z

...lembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/test_smoke.py

+    # Set up database to version 7b139906ac46
+    engine, script = prepared_schema_from_migrations_left
+
+    # Check that views have been added


This comment needs revising. Copy/pasta?

rod-glover · 2022-01-26T00:19:21Z

...lembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/test_smoke.py

+    engine, script = prepared_schema_from_migrations_left
+
+    # Check that views have been added
+    table = inspect(engine).get_columns("meta_vars", schema=schema_name)


Excellent use of inspection to check the migration action. 💯

rod-glover · 2022-01-26T00:25:44Z

...lembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/test_smoke.py

+
+@pytest.mark.usefixtures("new_db_left")
+def test_upgrade(prepared_schema_from_migrations_left, schema_name):
+    """test migration from 3d50ec832e47 to 7b139906ac46. """


We also need a check of the data migration action. That would look like the following:

Migrate to 3d50ec832e47

Load meta_vars with rows containing nulls in the migration columns. You can probably do that with a few inserts just to meta_vars, rather than loading all kinds of stuff as the big behavioural tests do.

Migrate to 7b139906ac46

Check the effects of the data migration

Check the effects of the schema migration (already coded)

You can find an example of this kind of two-stage upgrade test in tests/alembic_migrations/versions/v_e688e520d265_refactor_flags_association_tables. The way the initial target revision for test_upgrade is set up is subtle (a synonym for confusing): It is overridden by the parametrization

@pytest.mark.parametrize( "prepared_schema_from_migrations_left", ("bdc28573df56",), indirect=True )

of that method. You can do the same, except using of course the prior revision to this one.

If you get too stuck or confused, don't hesitate to reach out for help.

rod-glover · 2022-01-26T00:26:20Z

...lembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/test_smoke.py

+def test_downgrade(
+    prepared_schema_from_migrations_left, alembic_config_left, schema_name
+):
+    """Test the schema migration from 7b139906ac46 to 3d50ec832e47. """


And likewise a check of the downgrade data migration.

For this one there is a call of command.downgrade(), which I think means that I am testing the downgrade data migration. As far as not testing the upgrade though, I will add that in. I followed the views tests, as you noticed by my copy paste mistake, and that one did not run command.upgrade() in the test_upgrade feature, so that's why it is the way it is for me

Yes, this means you are testing the downgrade migration -- both schema and data.

For testing the upgrade, see my remarks above.

rod-glover

Keep at it, @helfy18 , it's coming along well.

pycds/alembic/versions/7b139906ac46_add_not_null_constraint_on_meta_vars.py

rod-glover · 2022-01-26T17:39:47Z

...lembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/test_smoke.py

+def test_downgrade(
+    prepared_schema_from_migrations_left, alembic_config_left, schema_name
+):
+    """Test the schema migration from 7b139906ac46 to 3d50ec832e47. """


Yes, this means you are testing the downgrade migration -- both schema and data.

rod-glover · 2022-01-26T17:40:10Z

...lembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/test_smoke.py

+def test_downgrade(
+    prepared_schema_from_migrations_left, alembic_config_left, schema_name
+):
+    """Test the schema migration from 7b139906ac46 to 3d50ec832e47. """


For testing the upgrade, see my remarks above.

rod-glover · 2022-01-26T17:46:49Z

...lembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/test_smoke.py

+
+@pytest.mark.usefixtures("new_db_left")
+def test_upgrade(prepared_schema_from_migrations_left, schema_name):
+    """test migration from 3d50ec832e47 to 7b139906ac46. """


You can find an example of this kind of two-stage upgrade test in tests/alembic_migrations/versions/v_e688e520d265_refactor_flags_association_tables. The way the initial target revision for test_upgrade is set up is subtle (a synonym for confusing): It is overridden by the parametrization

@pytest.mark.parametrize( "prepared_schema_from_migrations_left", ("bdc28573df56",), indirect=True )

of that method. You can do the same, except using of course the prior revision to this one.

If you get too stuck or confused, don't hesitate to reach out for help.

rod-glover

A bit more to do.

rod-glover · 2022-01-26T21:28:36Z

...lembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/test_smoke.py

+    )
+
+    command.upgrade(alembic_config_left, "+1")
+


After the migration check that the nulls in the row(s) you inserted have been set to the expected non-null values (or just not null).

rod-glover · 2022-01-26T21:30:02Z

...lembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/test_smoke.py

+    """Test the schema migration from 7b139906ac46 to 3d50ec832e47. """
+
+    # Set up database to version 7b139906ac46
+    engine, script = prepared_schema_from_migrations_left


Please add the downgrade data migration check: non-null values to nulls.

rod-glover · 2022-01-27T19:55:12Z

.../alembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/conftest.py

+def sesh_with_large_data(prepared_schema_from_migrations_left):
+    engine, script = prepared_schema_from_migrations_left
+    sesh = sessionmaker(bind=engine)()
+    insert_crmp_data(sesh)


If you omit this you will have a clean empty database. That's probably desirable for this test.

so remove this entire function from conftest.py?

rod-glover · 2022-01-27T19:56:50Z

...lembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/test_smoke.py

+
+    # Check that data has been modified to remove nulls
+    result = engine.execute(
+        f"SELECT * FROM {schema_name}.meta_vars"


To make comparisons simpler (see below), specify the columns explicitly here.

rod-glover · 2022-01-27T19:58:04Z

...lembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/test_smoke.py

+    result = engine.execute(
+        f"SELECT * FROM {schema_name}.meta_vars"
+    )
+    for row in result:


If the database starts out empty, you can assume that only the one row you inserted comes back. Try

row = next(result)

instead of the for loop.

And then you can check assert row == (....) where the parens are a tuple.

rod-glover · 2022-01-27T19:58:40Z

...lembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/test_smoke.py

+    engine.execute(
+        f"INSERT INTO {schema_name}.meta_vars(vars_id, cell_method, standard_name, display_name)"
+        f"VALUES (10000, 'foo: bar', 'foo_bar', 'foo bar')"
+    )


rod-glover · 2022-01-27T19:58:56Z

...lembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/test_smoke.py

+    result = engine.execute(
+        f"SELECT * FROM {schema_name}.meta_vars"
+    )
+    for row in result:


See comments for test_upgrade.

rod-glover

Looks great, @helfy18 ! Ready to merge!

rod-glover · 2022-01-27T23:24:23Z

...lembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/test_smoke.py

+    )
+
+    row = next(result)
+    assert row == (1, "foo: bar", "foo_bar", "foo bar")


rod-glover · 2022-01-27T23:24:49Z

...lembic_migrations/versions/v_7b139906ac46_add_not_null_constraint_on_meta_vars/test_smoke.py

+    )
+
+    row = next(result)
+    assert row == (10000, None, None, None)


rod-glover

Approved!

helfy18 added 5 commits January 20, 2022 11:18

Add not null constraint to table

4a7b0eb

Add not null constraints to three variables in the meta_vars table modify the subset data so that the constraint is not violated

Add alembic migration

2aa7beb

Modify alembic migration

0287df1

update migration

3ccc642

update tests for not-null compatibility

16213ce

helfy18 force-pushed the i29-add-not-null-constraint branch from 8299fcb to 16213ce Compare January 21, 2022 19:29

rod-glover reviewed Jan 21, 2022

View reviewed changes

helfy18 added 4 commits January 24, 2022 08:57

Run Black on files to clean formatting

9efce98

add migration tests

826ab1f

reset smoke test

a3953ce

remove swap file

29ed90c

rod-glover requested changes Jan 26, 2022

View reviewed changes

rod-glover reviewed Jan 26, 2022

View reviewed changes

helfy18 added 2 commits January 26, 2022 11:27

add data migration test to upgrade

742b145

run black on migration

cc786dc

rod-glover reviewed Jan 26, 2022

View reviewed changes

add test of data migration

7fd7bfd

rod-glover requested changes Jan 27, 2022

View reviewed changes

make tests simpler

77f9e62

rod-glover reviewed Jan 27, 2022

View reviewed changes

rod-glover approved these changes Jan 27, 2022

View reviewed changes

helfy18 merged commit 0a8e9cd into master Jan 27, 2022

helfy18 deleted the i29-add-not-null-constraint branch January 27, 2022 23:31

rod-glover changed the title ~~I29 add not null constraint~~ Add NOT NULL constraints to certain Variable (meta_vars) columns Jan 31, 2022

rod-glover changed the title ~~Add NOT NULL constraints to certain Variable (meta_vars) columns~~ Revision 7b139906ac46: Add NOT NULL constraints to certain Variable (meta_vars) columns Nov 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revision 7b139906ac46: Add NOT NULL constraints to certain Variable (meta_vars) columns #111

Revision 7b139906ac46: Add NOT NULL constraints to certain Variable (meta_vars) columns #111

helfy18 commented Jan 19, 2022 •

edited by rod-glover

Loading

rod-glover left a comment •

edited

Loading

rod-glover left a comment

rod-glover Jan 26, 2022

rod-glover Jan 26, 2022

rod-glover Jan 26, 2022 •

edited

Loading

rod-glover Jan 26, 2022

rod-glover Jan 26, 2022

helfy18 Jan 26, 2022

rod-glover Jan 26, 2022

rod-glover Jan 26, 2022

rod-glover left a comment

rod-glover Jan 26, 2022

rod-glover Jan 26, 2022

rod-glover Jan 26, 2022

rod-glover left a comment

rod-glover Jan 26, 2022

rod-glover Jan 26, 2022

rod-glover Jan 27, 2022

helfy18 Jan 27, 2022

rod-glover Jan 27, 2022

rod-glover Jan 27, 2022

rod-glover Jan 27, 2022

rod-glover Jan 27, 2022

rod-glover left a comment

rod-glover Jan 27, 2022

rod-glover Jan 27, 2022

rod-glover left a comment

Revision 7b139906ac46: Add NOT NULL constraints to certain Variable (meta_vars) columns #111

Revision 7b139906ac46: Add NOT NULL constraints to certain Variable (meta_vars) columns #111

Conversation

helfy18 commented Jan 19, 2022 • edited by rod-glover Loading

rod-glover left a comment • edited Loading

Choose a reason for hiding this comment

rod-glover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rod-glover Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rod-glover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rod-glover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rod-glover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rod-glover left a comment

Choose a reason for hiding this comment

helfy18 commented Jan 19, 2022 •

edited by rod-glover

Loading

rod-glover left a comment •

edited

Loading

rod-glover Jan 26, 2022 •

edited

Loading