-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Use daff for diff formatting in unit testing #8984
Use daff for diff formatting in unit testing #8984
Conversation
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. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## unit_testing_feature_branch #8984 +/- ##
===============================================================
- Coverage 86.80% 86.78% -0.03%
===============================================================
Files 181 181
Lines 27057 27089 +32
===============================================================
+ Hits 23488 23508 +20
- Misses 3569 3581 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@MichelleArk This is super useful to compare and contrast some of our formatting options! SummaryWhen kooking at a larger unit test case, it's possible to pick out the differences when using a colorized daff (data diff). But when looking at a naive tabular output, I wasn't able to discern the differences between the actual and expected. Non-colorized daff is much harder to see than a colorized daff. But it is still easier that the naive tabular output due to:
Our most readable formats may have similar characteristics as well. See below for more details. Crux of the issueThe crux of the issue is being able to determine two things:
Relevant factorsIt will be interesting to see how the following affects our ability to put our finger on what the actual diffs are:
2 columns by 10 rows; 2 differencesHere's a "bigger" example to compare/contrast with. Would be useful if we can up up with even more representative examples.
|
Overall I think the daff-based diff is a definite improvement over the naive tabular diff that's current implemented. It's much clearer to see what the diff actually is - especially with coloring :) My only concern is that the daff package has not had a python release in a while (last release was in 2020). The project itself seems to be active and supports many languages, and there is an issue open to release a more recent python package. It looks relatively stable though, only a handful of patch releases have gone out since aug 2020 for other supported languages. Otherwise - the stdout diff feels like an interface we can continue to change if we wanted to between minor versions. What's important is to standardize on a diff output in the json artifact that can be stable and consistent between different visualization implementations for the initial release though. cc @gshank - how does that sound to you in terms of proceeding here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! And it has the side benefit of giving us a reason to have variables and methods named "daff_diff" :-)
* 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 #8558
Problem
The diff is pretty lackluster for unit tests when they fail!
Solution
daff
to determine and render the diff betweenactual
andexpected
daff
rendering ifNO_COLORS
option is specifiedUnitTestDiff
in the result to include more structure - the actual json rows, expected json rows, and rendered diff.Note: There are quite a few moving pieces on this branch right now. I think we should add tests for unit test artifacts but it may make more sense to do once they are integrated into build/test commands since that will affect the results.
Checklist
🎩