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
test: snapshots for content tests #291
test: snapshots for content tests #291
Conversation
- Render - GetContent - AssertContains
Hi @TobiaszCudnik, the fix in testza will be released today, thanks 😉 Update: The fix in testza is now included in v0.2.12. To the snapshot testing: func TestBoxPrinter_WithTitleWithTitleTopCenter(t *testing.T) {
p := pterm.BoxPrinter{}
p2 := p.WithTitleTopCenter().WithTitle("a").Sprint("Lorem Ipsum")
testza.AssertContains(t, p2, "Lorem Ipsum")
testza.SnapshotCreateOrValidate(t, t.Name(), p2)
} I think Snapshots only make sense when complicated terminal output has to be checked (as for terminal output with color codes in particular). In the example above, the name assertion can be hardcoded ( I would say we snapshot every terminal output, like Feel free to ask questions anytime and thanks for working on this :) |
- removed snapshots for `AssertContains` - bumped `testza` to v0.2.12 from upstream - added snapshots for every step of the demo
Changes:
The motivation behind snapshotting I agree that snapshots have the biggest advantage with complicated output, thats why I reused the demo code and created an acceptance test, which has a snapshot on each step. Lesson learned: date generation doesnt work well with snapshots. Neither do random numbers. Unfortunately, BarChart seems to have OS-dependent output and fails on the CI, eg
No problem ;) |
Sadly I can't tell you why the BarChart has different output. I'll have to investigate that :) |
Hey @TobiaszCudnik, you might want to merge the current master branch into yours. The blank lines when running the tests, are now omitted, so the output looks cleaner. |
Ive merged the master and regenerated the snapshots, but I see that even on the new ubuntu CI youve added the output is still different in some cases (barchart and csv table). Its always shorter on the CI, but Ive checked it on macos and all snapshots pass. Maybe its related to the terminal being "dumb" on the CI, maybe not 256 color? |
My strong guess is that the detected terminal size for ubuntu is different. PTerm tries to fit the output to the terminal size. If the size cannot be detected, it uses a default value of 80 characters length. The Windows output for the BarChart is 80 chars long while the one from ubuntu is 60 chars long. You could try to set the terminal size manually to 80 inside the tests, or in the test initialization. I guess that would fix the problem. Let me know if it helps. |
Codecov Report
@@ Coverage Diff @@
## master #291 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 23 23
Lines 1306 1318 +12
=========================================
+ Hits 1306 1318 +12
Continue to review full report at Codecov.
|
I've commented out those tests with a note and created #296 to track it. This PR is ready to be merged, as other snapshots seem to work just fine. |
It indeed was the size detection, I have a PR with hardcoded values which passes. I will introduce a new API to force-set the dimensions. |
Hi @TobiaszCudnik, I see that you use |
- SetForcedTerminalSize() - CI tests fixed - optimized Render() & Srender()
- NewDefaultParagraph() - TestGetTerminalSizeAutodetect - docs
@MarvinJWendt This was a lot more shaving than I initially expected :) Terminal size was hardcoded on modules' inits. Changes:
Initially I didnt want to alter existing tests in any way, but I do agree that it should be optimized. |
Woah, that's a lot! I really like the work, but you introduced a breaking change by removing I see the difficulty in refreshing the terminal size on the printers and why it's important, tho. I guess we could introduce another option to the BTW: Huge thanks for all your work, this really helps a lot 😄 |
Changes:
Valid point regarding breaking APIs, especially in a test-related PR. I guess a proper resize support is out of scope here, but having a consistent policy regarding terminal size would be nice. Currently some values are hardcoded and some calculated during a render (eg grep For now Ive, reverted all the changes to the existing APIs and simply added |
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.
I have commented some files, other than that, the PR is ready in my eyes :)
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.
LGTM! Thanks for the contribution 🚀
Description
Snapshots for various content-related tests (35):
Render()
GetContent()
AssertContains()
Not covered:
...NilPrint()
test?Print??Contains()
Also skipped for now are tests without assertions, eg
TestAreaPrinter_RemoveWhenDone
. It may be a good idea to cover them as well.TODO - Depends on fixed testza snapshots. Currently theres a local module replace in
go.mod
, which has to be removed before merging.Scope
Related Issue
Fixes #246
To-Do Checklist