Enable inline csv format in unit testing #8743
Conversation
gshank
requested review from
thisispvb,
McKnight-42 and
aranke
and removed request for
a team
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## unit_testing_feature_branch #8743 +/- ##
===============================================================
+ Coverage 86.66% 86.69% +0.02%
===============================================================
Files 178 178
Lines 26234 26276 +42
===============================================================
+ Hits 22737 22779 +42
Misses 3497 3497
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
core/dbt/parser/unit_tests.py
Outdated
| if test_case.expect.format == "dict": | ||
| if isinstance(test_case.expect.rows, List): | ||
| expected_rows = test_case.expect.rows | ||
| else: | ||
| raise ParsingError("Wrong format for expected rows") | ||
| else: # test_case.expect.format == "csv": | ||
| # build a dictionary from the csv string | ||
| if isinstance(test_case.expect.rows, str): | ||
| expected_rows = self._build_rows_from_csv(test_case.expect.rows) | ||
| else: | ||
| raise ParsingError("Wrong format for expected rows") |
There was a problem hiding this comment.
I'm brainstorming ways that we could consolidate this logic and share it across InputFixture and OutputFixture. The simplest thing I can think of is adding a parsed_rows property on a shared UnitTestFixture base class that does this conditional logic to parse the raw rows given a format and convert it into a consistent internal representation. What do you think?
There was a problem hiding this comment.
An alternative could be to have a UnitTestFixture class that can be created from either an InputFixture or OutputFixture (making these just the external-facing interfaces). That way we could consolidate the logic of parsing the rows based on the format, into a consistent internal fixture object and store the parsed rows as state on that object.
There was a problem hiding this comment.
Yeah, I thought it would be a good idea to have some kind of combined logic but when I wrote this my brain was kind of offline :). Since we know we're going to also support csv files soon, it should be a solution that makes that easy. Let me think about whether a base class or derived class makes more sense...
| @property | ||
| def format(self) -> UnitTestFormat: | ||
| return UnitTestFormat.Dict | ||
|
|
||
| @property | ||
| def rows(self) -> Union[str, List[Dict[str, Any]]]: | ||
| return [] |
There was a problem hiding this comment.
is it necessary for these to be properties? Could they instead be attributes that are inherited by UnitTestInputFixture and UnitTestOutputFixture?
e.g.
rows: Union[str, List[Dict[str, Any]]] = ""
format: UnitTestFormat = UnitTestFormat.DictThere was a problem hiding this comment.
If we do that then we run into the frustrating issue of fields without defaults can't come after fields with defaults issue and have to split them out into a special class and do a different order. I'd kind of rather not.
* 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 #8626
Problem
Users want to be able to use the csv format in addition to dictionaries in their unit tests.
Solution
Add a "format" field to the given and expect structure, create an OutputFixture class (in addition to InputFixture).
Checklist