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

Feature: defer tests #2954

Merged
merged 2 commits into from
Dec 17, 2020
Merged

Feature: defer tests #2954

merged 2 commits into from
Dec 17, 2020

Conversation

jtcohen6
Copy link
Contributor

resolves #2701
picks up from #2706
requires #2946

Description

There's a lengthy section of the state comparison caveats devoted solely to tests. To make a long story short:

  • If a test is new or modified, but its parent model is not, dbt run -m state:modified && dbt test -m state:modified will return a failure because we will execute the test without having built its parent
  • If a test has multiple parents, but only one of those parent models is new or modified, the same commands will return a failure because we will execute the test without having built one of its parents
  • and so on.

Previously, I have recommended that community members use the advanced node selection syntax available in v0.18 to pick only the tests they'd want to run (--exclude test_name:relationships test_type:data), or to ensure that the parent models of modified tests were always built with commands like:

$ dbt run -m state:modified @state:modified,1+test_type:data
$ dbt test -m state:modified

In truth, everything would be a lot simpler if we added support for dbt test --defer. I believe we can do that, following a subtle-but-significant change to what deferral actually means (#2946). If a parent resource (seed, model, snapshot, etc) exists in the current namespace, we use it; if it doesn't exist in the current namespace, we defer its reference to the state manifest's namespace. (Inclusion or exclusion from the selection criteria has nothing to do with which objects are deferred, since models are never included by the criteria of dbt test.)

Let's say we're running a CI job, and comparing against a manifest from the main branch / production. Our model_b has been changed, model_a has not, and there's a relationships test between them.

Our CI job can now be very simple:

$ dbt run -m state:modified
$ dbt test -m state:modified

In the first step, we will build ci_schema.model_b because it has been modified. In the second step, we will execute the relationships test, which is selected because its parent model_b is modified. The compiled SQL for that test will look like:

select count(*) as validation_errors
from (
    select col3 as id from "db"."production"."model_a"
) as child
left join (
    select col3 as id from "db"."ci_schema"."model_b"
) as parent on parent.id = child.id
where child.id is not null
  and parent.id is null

Granted, I don't think relationships tests make a lot of sense in a CI context, and they make even less sense when trying to assert referential integrity across environments. That being said, the query will succeed and return a number of rows. It's then up to the user to decide if they want to update their selection criteria with --exclude test_name:relationships.

Checklist

  • 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 updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot

This comment has been minimized.

@jtcohen6 jtcohen6 force-pushed the feature/defer-tests branch from 6a27371 to 9d87535 Compare December 15, 2020 04:03
@cla-bot

This comment has been minimized.

@jtcohen6 jtcohen6 force-pushed the feature/defer-tests branch from 9d87535 to 8ed1f70 Compare December 15, 2020 04:05
@cla-bot cla-bot bot added the cla:yes label Dec 15, 2020
@drewbanin
Copy link
Contributor

I like this a lot, and I think it's such a fascinating reframing of the original problem. I buy the argument you're laying out here, and am highly supportive of our answer for CI being:

$ dbt run -m state:modified
$ dbt test -m state:modified

instead of

$ dbt run -m state:modified @state:modified,1+test_type:data
$ dbt test -m state:modified

I think I need to go deeper on some of these deferral mechanics, but I wonder if there are new UX-things we should be doing to help clarify what, actually, is happening in a given model/test execution. That could very well be a topic for consideration in the "Improve testing UX" conversation. Just flagging that dbt's CLI output used to map 1-to-1 with objects created or tested in the database, but now, a given test could actually run queries that join across dev and prod schemas. That's A Feature, but I do want to make sure that we're appropriately representing it to users and creating the opportunity for effective debugging if things go wrong.

Just throwing it out there, happy to discuss further or shelf it for the future. For the moment though, really like this approach

@jtcohen6 jtcohen6 force-pushed the feature/defer-tests branch from 8ed1f70 to 274c301 Compare December 16, 2020 19:53
@jtcohen6
Copy link
Contributor Author

I wonder if there are new UX-things we should be doing to help clarify what, actually, is happening in a given model/test execution.

This is a really fair point. Currently, we have a debug-level log message to enumerate which nodes have been deferred:
https://github.com/fishtown-analytics/dbt/blob/aff72996a18f09be49c5aa9ae775cdf90f03b5af/core/dbt/contracts/graph/manifest.py#L916-L918

Merged 5 items from state (sample: ['model.testy.table_model', 'seed.testy.bad_seed', 'seed.testy.my_seed', 'seed.testy.seed', 'seed.testy.other_seed'])

We could update the wording, and make it log to stdout instead.

@jtcohen6 jtcohen6 marked this pull request as ready for review December 16, 2020 23:03
@jtcohen6 jtcohen6 requested review from gshank and kwigley December 16, 2020 23:04
@drewbanin
Copy link
Contributor

Yeah - I'm almost picturing that those nodes show up in the stdout logs as though we were running them, but they have a status like DEFERRED or UPSTREAM. Not advocating for that in particular - I bet there are reasons why we would not want to do it - but there's definitely some magic happening in here that will be supremely confusing to someone who does not possess complete information of the state of multiple different database schemas at the point in time when a run occurs :D

Copy link
Contributor

@kwigley kwigley left a comment

Choose a reason for hiding this comment

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

👍

@jtcohen6
Copy link
Contributor Author

Thanks for the review, all. I spent a decent chunk of time today rewriting docs to account for these functional changes (dbt-labs/docs.getdbt.com#488).

Separately, let's continue discussing cosmetic changes:

  • Should we still call this defer?
  • How can we make it clear to users which models/resources have been deferred, and which haven't?

@jtcohen6 jtcohen6 merged commit adae512 into dev/kiyoshi-kuromiya Dec 17, 2020
@jtcohen6 jtcohen6 deleted the feature/defer-tests branch December 17, 2020 23:01
@drewbanin
Copy link
Contributor

@jtcohen6 you want to open up a new issue for those discussions? Feels like they're important ones, and probably doesn't make sense to relegate them to the bottom of this PR!

@jtcohen6 jtcohen6 mentioned this pull request Dec 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support deferred tests
4 participants