Skip to content
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

Refine test assertion output #423

Merged
merged 1 commit into from Sep 5, 2023
Merged

Conversation

scothis
Copy link
Contributor

@scothis scothis commented Aug 31, 2023

  • allow colorized diffs, use COLOR_DIFF=true to force on
  • clarify what is unexpected or missing
  • reference specific expected field and index when appropriate
  • hide "for config X" for the default config, continue to show for extra configs

- allow colorized diffs, use `COLOR_DIFF=true` to force on
- clarify what is unexpected or missing
- reference specific expected field and index when appropriate
- hide "for config X" for the default config, continue to show for extra
  configs

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Patch coverage: 79.59% and project coverage change: +0.11% 🎉

Comparison is base (a688686) 60.78% compared to head (3be702e) 60.89%.

❗ Current head 3be702e differs from pull request most recent head f98799a. Consider uploading reports for the commit f98799a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #423      +/-   ##
==========================================
+ Coverage   60.78%   60.89%   +0.11%     
==========================================
  Files          25       26       +1     
  Lines        2494     2514      +20     
==========================================
+ Hits         1516     1531      +15     
- Misses        893      896       +3     
- Partials       85       87       +2     
Files Changed Coverage Δ
testing/reconciler.go 0.00% <0.00%> (ø)
testing/subreconciler.go 0.00% <0.00%> (ø)
testing/webhook.go 0.00% <0.00%> (ø)
testing/color.go 81.25% <81.25%> (ø)
testing/config.go 86.93% <92.85%> (-0.62%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

)

func init() {
if _, ok := os.LookupEnv("COLOR_DIFF"); ok {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[issue, suggestion]: I get a coloured diff even if COLOR_DIFF is not set. It seems that COLOR_DIFF does not override faith/color's https://github.com/fatih/color#disableenable-color mechanics. Since faith/color is already being clever depending on the connected output, couldn't we drop COLOR_DIFF and let the fatih/color handle it? After all, NO_COLOR would be a way to disable it.

Copy link
Contributor Author

@scothis scothis Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, while testing I could never get the color to be active by default. I assumed there was something about go test is executing that triggered the default to no color. I added COLOR_DIFF to force color on.

How are you executing the code? Version of go, os, console.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❯ go version
go version go1.21.0 darwin/amd64

❯ uname -a
Darwin ***** 22.6.0 Darwin Kernel Version 22.6.0: Wed Jul  5 22:21:56 PDT 2023; root:xnu-8796.141.3~6/RELEASE_X86_64 x86_64

❯ # terminal
❯ kitty --version
kitty 0.29.2 created by Kovid Goyal

❯ # run tests
❯ go run github.com/onsi/ginkgo/v2/ginkgo --keep-going -r
...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see colorized output by default with both kitty and ginkgo, but using either a different test runner or a different terminal and it's all plain text.

NO_COLOR is only used to disable color output. NO_COLOR=true and NO_COLOR=false are equivalent ways to disable color output. I'm using COLOR_DIFF as a way to override the tty detection and force colorized output. I'm up for ways to be smarter about the default detection.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scothis Thanks for running the experiments and making the call. Yay colors!

@scothis scothis merged commit ce2ead6 into vmware-labs:main Sep 5, 2023
4 checks passed
@scothis scothis deleted the color-diff branch September 5, 2023 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants