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

In build command run unit tests before models #9273

Merged
merged 24 commits into from
Dec 20, 2023

Conversation

gshank
Copy link
Contributor

@gshank gshank commented Dec 12, 2023

resolves #9128

Problem

In the build command we want to be able to run unit tests first, before the model it tests, and skip the model if a unit test fails.

For selection purposes we want to keep the graph with the unit tests depending on the model. But for running the build command we want the opposite.

Solution

There were two alternatives for a solution here, and both of them have difficulties.

The most natural would be to change the graph to have the unit tests before the model, but that would be very strange for selection semantics, so we would have to have two separate dags, one with unit tests after the model and another with unit tests before the model. We could have modified the selection dag to look like the required execution dag, but that would be fairly complicated since every unit test would have to be inserted as parent of the model and itself be a child of every one of the model's parents. There were also concerns about performance, since adding test_edges to the build dag made things slower, and this solution would have increased the size of the dag even more.

The other alternative, and the one that's implemented here, is to insert the execution of the unit tests in before the execution of the model. This requires some refactoring of the way we "run" the job_queue, and some somewhat inelegant hacking of the build code. But is probably easier to understand than the graph hacking solution and doesn't have the performance risks.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • 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
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX
  • This PR includes type annotations for new and modified functions

gshank and others added 12 commits November 28, 2023 09:51
* Rm --dry-run flag for dbt deps

* Add changelog entry

* Update test

* PR feedback
* init attempt of adding clean_up methods to basic and unique_id tests

* swapping cleanup method drop of test_schema to unique_schema to test breakage on docs_generate test

* moving the clean_up method down into class BaseDocsGenerate

* remove drop relation for unique_schema

* manually define alternate_schema for clean_up as not being seen as part of project_config

* add changelog

* remove unneeded changelog

* uncomment line that generates new manifest and delete manifest our changes created

* make sure the manifest test is deleted and readd older version of manifest.json to appease test

* manually revert file to previous commit

* Revert "manually revert file to previous commit"

This reverts commit a755419.
@gshank gshank requested a review from a team as a code owner December 12, 2023 20:15
@cla-bot cla-bot bot added the cla:yes label Dec 12, 2023
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4e87f46) 85.64% compared to head (9340dae) 86.90%.

Files Patch % Lines
core/dbt/task/build.py 97.43% 2 Missing ⚠️
Additional details and impacted files
@@                       Coverage Diff                       @@
##           unit_testing_feature_branch    #9273      +/-   ##
===============================================================
+ Coverage                        85.64%   86.90%   +1.25%     
===============================================================
  Files                              181      181              
  Lines                            27274    27338      +64     
===============================================================
+ Hits                             23360    23758     +398     
+ Misses                            3914     3580     -334     
Flag Coverage Δ
integration 83.85% <97.97%> (+1.43%) ⬆️
unit 64.69% <22.22%> (-0.13%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gshank gshank requested review from a team as code owners December 13, 2023 18:00
@gshank gshank requested review from wpowers-dbt and removed request for a team December 13, 2023 18:00
@@ -30,7 +30,3 @@ services:
working_dir: /usr/app
depends_on:
- database

networks:
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this change intentional? What does it do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "docker compose up -d database" doesn't work because it complains about it. Not sure what it did to start with.

@gshank gshank merged commit a0177e3 into unit_testing_feature_branch Dec 20, 2023
49 checks passed
@gshank gshank deleted the 9128-unit_tests_before_models branch December 20, 2023 21:04
gshank added a commit that referenced this pull request Jan 16, 2024
* Initial implementation of unit testing (from pr #2911)

Co-authored-by: Michelle Ark <[email protected]>

* 8295 unit testing artifacts (#8477)

* unit test config: tags & meta (#8565)

* Add additional functional test for unit testing selection, artifacts, etc (#8639)

* Enable inline csv format in unit testing (#8743)

* Support unit testing incremental models (#8891)

* update unit test key: unit -> unit-tests (#8988)


* convert to use unit test name at top level key (#8966)

* csv file fixtures (#9044)

* Unit test support for `state:modified` and `--defer` (#9032)

Co-authored-by: Michelle Ark <[email protected]>

* Allow use of sources as unit testing inputs (#9059)

* Use daff for diff formatting in unit testing (#8984)

* Fix #8652: Use seed file from disk for unit testing if rows not specified in YAML config (#9064)

Co-authored-by: Michelle Ark <[email protected]>
Fix #8652: Use seed value if rows not specified

* Move unit testing to test and build commands (#9108)

* Enable unit testing in non-root packages (#9184)

* convert test to data_test (#9201)

* Make fixtures files full-fledged members of manifest and enable partial parsing (#9225)

* In build command run unit tests before models (#9273)

---------

Co-authored-by: Michelle Ark <[email protected]>
Co-authored-by: Michelle Ark <[email protected]>
Co-authored-by: Emily Rockman <[email protected]>
Co-authored-by: Jeremy Cohen <[email protected]>
Co-authored-by: Kshitij Aranke <[email protected]>
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.

4 participants