Feat: Refactor model test output [skip ci] #4668
Conversation
VaggelisD
force-pushed
the
vaggelisd/test_verbose_comparison
branch
2 times, most recently
from
45fedf6 to
75d8d9a
Compare
VaggelisD
changed the title
VaggelisD
force-pushed
the
vaggelisd/test_verbose_comparison
branch
from
75d8d9a to
55650d4
Compare
georgesittas
left a comment
georgesittas
left a comment
There was a problem hiding this comment.
I like this, curious to see what others have to say. Is it possible to point out the exact differences per different rows reported, similar to git diff?
| @abc.abstractmethod | ||
| def log_unit_test_results(self, result: ModelTextTestResult, test_duration: float) -> None: | ||
| """Print the unit test results.""" | ||
|
|
There was a problem hiding this comment.
Let's factor this out into a UnitTestingConsole.
sqlmesh/core/test/definition.py
Outdated
| default_catalog: str | None = None, | ||
| concurrency: bool = False, | ||
| verbosity: Verbosity = Verbosity.DEFAULT, | ||
| rich_output: bool = True, |
There was a problem hiding this comment.
Why do we need this option and not just default to rich output all the time?
There was a problem hiding this comment.
My issue with that had to do with test_test.py, we check the payload itself in the exception (_check_successful_or_raise) instead of the output. That used to be a str but is now a tuple of str | Rich.Table so the comparison fails.
By allowing the old style we can maintain the tests as they are e.g test_row_order
There was a problem hiding this comment.
Doesn't seem like a good argument to keep it. We should instead update the tests, imo, unless there's a compelling reason to expose a non-rich formatting option.
How does this look in formats like CI/CD bot, magics, etc?
I gave this a shot, Panda's compare has the Would be awesome if we can truly separate the equal values and keep the tables as diff only |
tests/core/test_test.py
Outdated
| " value ds \n" | ||
| " exp act exp act\n" | ||
| "0 None 2 None 3\n" | ||
| "AssertionError: Data mismatch\n\n" |
There was a problem hiding this comment.
now that you use rich check capture_console_output and strip_ansi_codes methods in test_table_diff.py so that the assertions strip the output of rich styling so that they can work in the tests. maybe if you end up using these they can be moved to some utils folder so that they can be shared
VaggelisD
force-pushed
the
vaggelisd/test_verbose_comparison
branch
2 times, most recently
from
b822086 to
acd65b1
Compare
VaggelisD
changed the title
VaggelisD
force-pushed
the
vaggelisd/test_verbose_comparison
branch
4 times, most recently
from
7653ce2 to
454437c
Compare
VaggelisD
force-pushed
the
vaggelisd/test_verbose_comparison
branch
from
454437c to
4b877d3
Compare
|
Still trying to figure out the smaller output details so will close this PR and reopen once those are cleaner (plus comments are addressed) |
Before this PR:
After this PR