Ct 866/migrate hook tests fixed #5760

VersusFacit · 2022-09-05T07:15:13Z

Description

This was supposed to a simple, humble test conversion. Instead, I fixed a weird timezone error that intermittently arises on snapshots tests. And I finally found the original root of the weird structured logging problem.

So what the heck happened in this PR

There was...a lot behind these code changes.

Without the presence of certain integration tests (but not others -- this I was never able to solve), DBT_LOG_FORMAT doesn't get set to json for pre-integration-test class instantiations. It shouldn't be NULL, but flags.py doesn't get invoked for certain integration test events....unless you do a secondary check.

You can see this happen when removing 014 or 015 and I presume many many others. When I removed test014, the structured log test broke because the internal FLAG was not being set that suppresses a handful of debug connection messages on each thread.

With this PR's ancillary check in place, the test (thank goodness) passes the structured logging check--that garbage data is no longer coughed up.

PRs #5705 #5654 were the poor PRs that I subjected extensive CI testing to when coaxing out exactly what was going on and how our CI differs from our local testing environments.

Checklist

I have read the contributing guide and understand what's expected of me
I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
I have run changie new to create a changelog entry

github-actions · 2022-09-05T07:15:39Z

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

core/dbt/events/functions.py

tests/functional/hooks/fixtures.py

VersusFacit · 2022-09-05T07:33:14Z

tests/functional/simple_snapshot/test_hard_delete_snapshot.py

@@ -24,6 +36,12 @@ def datetime_snapshot():
    return begin_snapshot_datetime


+@pytest.fixture(scope="class", autouse=True)
+def setUp(project):


was messing around in here and left this change as it better reflects our current best practices

VersusFacit · 2022-09-05T07:34:49Z

tests/functional/simple_snapshot/test_hard_delete_snapshot.py

@@ -79,7 +94,7 @@ def test_snapshot_hard_delete(project):
    for result in snapshotted[10:]:
        # result is a tuple, the dbt_valid_to column is the latest
        assert isinstance(result[-1], datetime)
-        assert result[-1].replace(tzinfo=pytz.UTC) >= invalidated_snapshot_datetime
+        assert result[-1].replace(tzinfo=currenttz()) >= invalidated_snapshot_datetime


this fixes an intermittent error that will show up in my (and others', judging from slack threads) local test invocations. I ran into, and fixed, this as part of getting my local CI to precisely match our remote CI

nathaniel-may

Great job finding these two VERY sneaky issues. I'm so happy we can finally put this to rest.

.changes/unreleased/Under the Hood-20220905-001749.yaml

core/dbt/events/functions.py

tests/functional/hooks/fixtures.py

nathaniel-may · 2022-09-06T14:32:54Z

tests/functional/simple_snapshot/test_hard_delete_snapshot.py

+def currenttz():
+    if time.daylight:
+        return timezone(timedelta(seconds=-time.altzone), time.tzname[1])
+    else:
+        return timezone(timedelta(seconds=-time.timezone), time.tzname[0])


I suspect there might be a better way to do this, but I'm find merging this if it's not obvious.

Honestly, it's hard to find an answer in the std lib. I went with this to avoid adding a third party library. Not tied to any which way though.

gshank

I'm glad you got this working, but I don't think the solution is right yet. I think we need to be calling flags.set_from_args someplace.

gshank · 2022-09-06T15:20:32Z

core/dbt/events/functions.py

+    # Flags is not set in the integration test suite but is by the environment.
+    # DBT_LOG_FORMAT is used in our test suite so checking for it's presence is key.
+    this.format_json = (
+        flags.LOG_FORMAT == "json" or os.environ.get("DBT_LOG_FORMAT", "json") == "json"


I don't think that this is correct. If the log_format isn't set by env_var, user_config or cli option, the default is None. This is basically changing the default to be 'json'. If the log_format isn't set and it IS set in an env_var, it means that flags.set_from_args has not been called someplace where it ought to be.

This is basically changing the default to be 'json'.

Yep, that was to get this working, and I think I mentioned (or at least intended to) this was just my naive solution after identifying the problem.

If the log_format isn't set and it IS set in an env_var, it means that flags.set_from_args has not been called someplace where it ought to be.

See, this is exactly where I was hoping someone more familiar would know the answer. Let me see about getting that routine called and reverting this line.

Think I've got something better now. Best part is once the base.py is removed, this patch will go with it, i.e. the fix is scoped to integration tests

…to check for environment variables using flag routine.

gshank

In theory, there might be a better place to put the flags.set_from_args call (including the user_config and args params), but integration/base.py is pretty convoluted and as you say is going to go away. So I'm fine with this as a way to move forward :)

* Finish converting first test file. * Finish test conversion. * Remove old integration hook tests. * Move location of schema.yml to models directory. * fix snapshot delete test that was failing * Add the extra env var check for our CI. * Add changelog * Remove naive json flag check and instead force all integration tests to check for environment variables using flag routine. * Revise the changelog to be more of an explanation. Co-authored-by: Mila Page <versusfacit@users.noreply.github.com>

VersusFacit added 8 commits September 5, 2022 00:06

Finish converting first test file.

e732e46

Finish test conversion.

d6447aa

Add other test (wasn't staged).

9c1e8d7

Remove old integration hook tests.

a5e176a

Move location of schema.yml to models directory.

3a32d5f

It couldn't be the yaml could it

dd8d713

fix snapshot delete test that was failing

6b08255

Add the extra env var check for our CI.

4806f7d

cla-bot bot added the cla:yes label Sep 5, 2022

Add changelog

81c7610

VersusFacit commented Sep 5, 2022

View reviewed changes

core/dbt/events/functions.py Outdated Show resolved Hide resolved

VersusFacit commented Sep 5, 2022

View reviewed changes

tests/functional/hooks/fixtures.py Show resolved Hide resolved

VersusFacit commented Sep 5, 2022

View reviewed changes

VersusFacit marked this pull request as ready for review September 5, 2022 07:49

VersusFacit requested review from a team as code owners September 5, 2022 07:49

VersusFacit requested review from nathaniel-may and McKnight-42 September 5, 2022 07:49

VersusFacit self-assigned this Sep 5, 2022

nathaniel-may approved these changes Sep 6, 2022

View reviewed changes

gshank requested changes Sep 6, 2022

View reviewed changes

VersusFacit added 2 commits September 6, 2022 10:55

Remove naive json flag check and instead force all integration tests …

b39b974

…to check for environment variables using flag routine.

Revise the changelog to be more of an explanation.

36a09c1

VersusFacit requested a review from gshank September 6, 2022 18:56

gshank approved these changes Sep 6, 2022

View reviewed changes

VersusFacit merged commit 0f6e4f0 into main Sep 6, 2022

VersusFacit deleted the CT-866/migrate_hook_tests_fixed branch September 6, 2022 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ct 866/migrate hook tests fixed #5760

Ct 866/migrate hook tests fixed #5760

VersusFacit commented Sep 5, 2022 •

edited

Loading

github-actions bot commented Sep 5, 2022

VersusFacit Sep 5, 2022

VersusFacit Sep 5, 2022 •

edited

Loading

nathaniel-may left a comment

nathaniel-may Sep 6, 2022

VersusFacit Sep 6, 2022 •

edited

Loading

gshank left a comment

gshank Sep 6, 2022

VersusFacit Sep 6, 2022 •

edited

Loading

VersusFacit Sep 6, 2022

gshank left a comment

Ct 866/migrate hook tests fixed #5760

Ct 866/migrate hook tests fixed #5760

Conversation

VersusFacit commented Sep 5, 2022 • edited Loading

Description

So what the heck happened in this PR

Checklist

github-actions bot commented Sep 5, 2022

VersusFacit Sep 5, 2022

Choose a reason for hiding this comment

VersusFacit Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

nathaniel-may left a comment

Choose a reason for hiding this comment

nathaniel-may Sep 6, 2022

Choose a reason for hiding this comment

VersusFacit Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

gshank left a comment

Choose a reason for hiding this comment

gshank Sep 6, 2022

Choose a reason for hiding this comment

VersusFacit Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

VersusFacit Sep 6, 2022

Choose a reason for hiding this comment

gshank left a comment

Choose a reason for hiding this comment

VersusFacit commented Sep 5, 2022 •

edited

Loading

VersusFacit Sep 5, 2022 •

edited

Loading

VersusFacit Sep 6, 2022 •

edited

Loading

VersusFacit Sep 6, 2022 •

edited

Loading