8295 unit testing artifacts #8477
Conversation
back if found necessary.
gshank
requested review from
MichelleArk
and removed request for
QMalcolm and
mikealfare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## unit_testing_feature_branch #8477 +/- ##
===============================================================
- Coverage 86.43% 86.42% -0.01%
===============================================================
Files 176 176
Lines 25914 25962 +48
===============================================================
+ Hits 22398 22438 +40
- Misses 3516 3524 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
|
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. |
tested, not working.
create the unit_test_manifest.
test_name selector
| @@ -31,6 +31,8 @@ def can_select_indirectly(node): | |||
| """ | |||
| if node.resource_type == NodeType.Test: | |||
| return True | |||
| elif node.resource_type == NodeType.Unit: | |||
| return True | |||
There was a problem hiding this comment.
My understanding is that this is to power selection for the unit test task, but does not actually include unit tests in other task execution because UnitTestNodes are not in the manifest. Is that correct?
There was a problem hiding this comment.
In this case the objects with NodeType.Unit are actually the UnitTestCases in manifest.unit_tests, which are in the manifest. They're not included in other tasks because each task has a Selector which filters out other resource types. So unit test cases aren't included in the "run" task job_queue because it only executes NodeType.Model.
| @@ -879,14 +879,12 @@ def test(ctx, **kwargs): | |||
| @requires.project | |||
| @requires.runtime_config | |||
| @requires.manifest | |||
| @requires.unit_test_collection | |||
There was a problem hiding this comment.
what is the reasoning for loading the unit test manifest (much prefer this naming to "collection"!) to the task as opposed to as a requires decorator?
There was a problem hiding this comment.
A requires decorator runs before the task starts. The unit test manifest can't be created until after selection is handled, which happens during task.run.
| if which and which == "unit-test": | ||
| file_name = UNIT_TEST_MANIFEST_FILE_NAME | ||
| else: | ||
| file_name = MANIFEST_FILE_NAME |
There was a problem hiding this comment.
not blocking but: i think this method would be more generalized and we'd plumb fewer high-level concerns (e.g. which command is run) through to lower-level components if the optional argument was file_name instead of which -- assuming its possible to plumb the UNIT_TEST_MANIFEST_FILE_NAME itself through from the requires decorator
There was a problem hiding this comment.
We don't need it in the write_manifest call in the requires decorator. We need it in the write_manifest call from runnable.py. So we'd just have to move the "if" to the runnable "run" method, which we could certainly do, but doesn't seem like that much of an improvement. And we'd still need an "if" in the write_manifest method to determine which file name to use unless we pass the file name in every place that we call "write_manifest" which feels like overkill.
* Initial implementation of unit testing (from pr #2911) Co-authored-by: Michelle Ark <michelle.ark@dbtlabs.com> * 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 <michelle.ark@dbtlabs.com> * 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 <MichelleArk@users.noreply.github.com> 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 <michelle.ark@dbtlabs.com> Co-authored-by: Michelle Ark <MichelleArk@users.noreply.github.com> Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com> Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: Kshitij Aranke <kshitij.aranke@dbtlabs.com>
resolves #8295
Problem
Produce artifacts in the manifest (or elsewhere) with metadata about existing unit tests.
Solution
A dictionary of UnitTestCases is stored in manifest.unit_tests.
Selection happens against a standard manifest (filtering for UnitTestCases in manifest.unit_tests), then the unit test manifest is built with UnitTestNodes in manifest.nodes plus input models.
This pull request also implements selection of a single unit test and use of "standard" selection syntax for models.
In addition it implements partial parsing for the manifest.unit_tests.
Checklist