Skip to content
This repository has been archived by the owner on Aug 28, 2024. It is now read-only.

Refine test assertion output #423

Merged
merged 1 commit into from
Sep 5, 2023
Merged

Refine test assertion output #423

merged 1 commit into from
Sep 5, 2023

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
1 check passed
@scothis scothis deleted the color-diff branch September 5, 2023 23:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants