test: snapshots for content tests

[TobiaszCudnik]

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

What is affected by this pull request?

• Bug Fix
• New Feature
• Documentation
• Other

Related Issue

Fixes #246

To-Do Checklist

• I tested my changes
• N/A I have commented every method that I created/changed
• N/A I updated the examples to fit with my changes
• N/A I have added tests for my newly created methods

[MarvinJWendt]

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:
I don't really know, if we should snapshot variable content (assertions) like in:

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 (testza.AssertContains(t, p2, "Lorem Ipsum")), so I don't think it needs a snapshot. Or do you have something in mind that I don't see? 😄

I would say we snapshot every terminal output, like Render() and Print functions, as they are hard to assert with the color codes. Everything that can be tested via simple assertions (eg.: variable content) does not need a snapshot IMO.

Feel free to ask questions anytime and thanks for working on this :)

[TobiaszCudnik]

Changes:

• removed snapshots for AssertContains
• bumped testza to v0.2.12 from upstream
• added snapshots for every step of the demo

The motivation behind snapshotting AssertContains was that it doesnt assert the whole output (like AssertEquals), but the snapshots would. On the other hand, snapshots are costly to maintain.

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 TestBarChartPrinter_RenderHorizonzal. Do you have an idea why?

Feel free to ask questions anytime and thanks for working on this :)

No problem ;)

[MarvinJWendt]

Sadly I can't tell you why the BarChart has different output. I'll have to investigate that :)
For now you could name the snapshot something like [test name] + "_" + runtime.GOOS, so that each platform has their own snapshot of the BarChart printer. But that's just a workaround, I'll take a look why the output differs.

[MarvinJWendt]

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.

[TobiaszCudnik]

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?

[MarvinJWendt]

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]

Codecov Report

Merging #291 (d9031f2) into master (da58249) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #291   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines         1306      1318   +12     
=========================================
+ Hits          1306      1318   +12     

Impacted Files	Coverage Δ
barchart.go	100.00% <ø> (ø)
pterm.go	100.00% <100.00%> (ø)
terminal.go	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da58249...d9031f2. Read the comment docs.

[TobiaszCudnik]

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.

[TobiaszCudnik]

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.

[MarvinJWendt]

Hi @TobiaszCudnik, I see that you use Srender() to get the content from the printers, and save this to the snapshots. Do you think it would make more sense to capture the output of the previous Render() call, and validate its output? If someone messes up the Render() function, the end-user won't get the correct output, and it won't be caught by the unit tests.

[TobiaszCudnik]

@MarvinJWendt This was a lot more shaving than I initially expected :) Terminal size was hardcoded on modules' inits.

Changes:

• SetForcedTerminalSize()
  • tests
• CI tests fixed
• optimized Render() & Srender() tests
• NewDefaultBarChart() constructor
  • updated docs & examples
• OS dependent-snapshots (some)
  • in-repo only for linux
• NewDefaultParagraph() constructor
  • updated docs & examples

I see that you use Srender() to get the content from the printers, and save this to the snapshots. Do you think it would make more sense to capture the output of the previous Render() call, and validate its output?

Initially I didnt want to alter existing tests in any way, but I do agree that it should be optimized.

[MarvinJWendt]

Woah, that's a lot! I really like the work, but you introduced a breaking change by removing pterm.DefaultBarChart, which also doesn't fit to the "paradigm" of PTerm to access every printer with DefaultXxx. Am I seeing this right, that you created a constructor, so that the terminal size gets reloaded when a new printer is created? I totally see the intention, but since the bar chart is widely used in other projects, it might not be an option to introduce a breaking change there (some bigger projects would not build anymore, if they update PTerm). Also, a problem is that you cannot modify the Default values of the BarChart anymore. Normally, a user can overwrite values of DefaultXxx to change the printer settings on a project level. With a constructor, this is no longer possible. See: https://pterm.sh/#/docs/customizing/override-default-printer
Especially for the BarChart, a user might want to change the actual bar character of the default printer.

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 BarChart and Paragraph, like FitToTerminalSize bool. If that value is true, the size get's updated before every re-render to fit the terminal. If that value is false, the size will be static. What do you think about that? If you have a better idea, tell me :)

---

BTW: Huge thanks for all your work, this really helps a lot 😄

[TobiaszCudnik]

Changes:

• reverted constructors
• added RecalculateTerminalSize

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 GetTerminalWidth()).

For now Ive, reverted all the changes to the existing APIs and simply added RecalculateTerminalSize(), which may also be useful for the end user. Although its manual, it will adapt new instances after a resize (which is exactly what was needed for the tests).

[MarvinJWendt]

I have commented some files, other than that, the PR is ready in my eyes :)

[MarvinJWendt]

LGTM! Thanks for the contribution 🚀