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

Use gotestsum instead of gotest #4850

Closed
wants to merge 0 commits into from

Conversation

khanhtc1202
Copy link
Contributor

@khanhtc1202 khanhtc1202 commented Apr 19, 2024

What type of PR is this?

/kind feature

What this PR does / why we need it:

I started playing with the Karmada source code today and make test is really hard to get what was failed with the size of the current test suite.
AFAIK, we also use the gotest tool to color the test output, but the key point is it's still hard to scroll up until I can reach the test failed (it's over my terminal scroll limit). Also, gotest is no longer maintained, and I am reading around for community-preferred solutions for this. I found out that gotestsum is quite a good choice. Some other big projects are using it (ref: their used by list), and it has been maintained well too.

With this PR changed, I can get a summary of which tests had failed at the end of the output like this (and it's colorful as well)

=== FAIL: pkg/util/helper TestGetMinTolerationTime/no_noExecuteTaints (unknown)
-1ns=== RUN   TestGetMinTolerationTime/no_usedTolerations
0s=== RUN   TestGetMinTolerationTime/with_noExecuteTaints_and_usedTolerations
59.999869458s=== RUN   TestGetMinTolerationTime/usedTolerantion.TolerationSeconds_is_nil
-1ns=== RUN   TestGetMinTolerationTime/noExecuteTaints.TimeAdded_is_nil
-1ns=== RUN   TestGetMinTolerationTime/find_the_latest_trigger_time
59.999728167s=== RUN   TestGetMinTolerationTime/trigger_time_is_up
0s--- PASS: TestGetMinTolerationTime (0.00s)
    --- PASS: TestGetMinTolerationTime/no_noExecuteTaints (0.00s)
    --- PASS: TestGetMinTolerationTime/no_usedTolerations (0.00s)
    --- PASS: TestGetMinTolerationTime/with_noExecuteTaints_and_usedTolerations (0.00s)
    --- PASS: TestGetMinTolerationTime/usedTolerantion.TolerationSeconds_is_nil (0.00s)
    --- PASS: TestGetMinTolerationTime/noExecuteTaints.TimeAdded_is_nil (0.00s)
    --- PASS: TestGetMinTolerationTime/find_the_latest_trigger_time (0.00s)
    --- PASS: TestGetMinTolerationTime/trigger_time_is_up (0.00s)

DONE 2321 tests, 4 failures in 63.560s
make: *** [test] Error 1

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 19, 2024
@karmada-bot
Copy link
Collaborator

Welcome @khanhtc1202! It looks like this is your first PR to karmada-io/karmada 🎉

@karmada-bot karmada-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 19, 2024
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.04%. Comparing base (c09ca0d) to head (3a6052c).
Report is 2 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4850      +/-   ##
==========================================
- Coverage   53.06%   53.04%   -0.02%     
==========================================
  Files         250      250              
  Lines       20371    20371              
==========================================
- Hits        10809    10806       -3     
- Misses       8844     8847       +3     
  Partials      718      718              
Flag Coverage Δ
unittests 53.04% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@RainbowMango
Copy link
Member

Hello Khanh, So excited to meet you here! Great thanks.

@RainbowMango
Copy link
Member

It looks great and I'll try it on my side.

But after a quick look, there is a failed case in the workflow, but it shows successful in the end. Do you know why?

@RainbowMango
Copy link
Member

cc @lxtywypc who might be interested in it.

@khanhtc1202
Copy link
Contributor Author

khanhtc1202 commented Apr 19, 2024

Hi @RainbowMango thanks for the warm greeting 😄

But after a quick look, there is a failed case in the workflow, but it shows successful in the end. Do you know why?

TBH, the test result returned as FAILED in my local even before I made this change, and it was the reason I adopted gotestsum in order to find out which test failed. Does the make test pass in your local?

@RainbowMango
Copy link
Member

RainbowMango commented Apr 20, 2024

I just ran a test with go test, gotest and gotestsum, the output of gotestsum is weird:

Output of go test, acts normally:

-bash-5.0# go test ./pkg/util/helper -run=GetMinTolerationTime -v
=== RUN   TestGetMinTolerationTime
=== RUN   TestGetMinTolerationTime/no_noExecuteTaints
-1ns=== RUN   TestGetMinTolerationTime/no_usedTolerations
0s=== RUN   TestGetMinTolerationTime/with_noExecuteTaints_and_usedTolerations
59.999896389s=== RUN   TestGetMinTolerationTime/usedTolerantion.TolerationSeconds_is_nil
-1ns=== RUN   TestGetMinTolerationTime/noExecuteTaints.TimeAdded_is_nil
-1ns=== RUN   TestGetMinTolerationTime/find_the_latest_trigger_time
59.999839202s=== RUN   TestGetMinTolerationTime/trigger_time_is_up
0s--- PASS: TestGetMinTolerationTime (0.00s)
    --- PASS: TestGetMinTolerationTime/no_noExecuteTaints (0.00s)
    --- PASS: TestGetMinTolerationTime/no_usedTolerations (0.00s)
    --- PASS: TestGetMinTolerationTime/with_noExecuteTaints_and_usedTolerations (0.00s)
    --- PASS: TestGetMinTolerationTime/usedTolerantion.TolerationSeconds_is_nil (0.00s)
    --- PASS: TestGetMinTolerationTime/noExecuteTaints.TimeAdded_is_nil (0.00s)
    --- PASS: TestGetMinTolerationTime/find_the_latest_trigger_time (0.00s)
    --- PASS: TestGetMinTolerationTime/trigger_time_is_up (0.00s)
PASS
ok  	github.com/karmada-io/karmada/pkg/util/helper	(cached)

Output of gotest, normal:

-bash-5.0# gotest ./pkg/util/helper -run=GetMinTolerationTime -v
=== RUN   TestGetMinTolerationTime
=== RUN   TestGetMinTolerationTime/no_noExecuteTaints
-1ns=== RUN   TestGetMinTolerationTime/no_usedTolerations
0s=== RUN   TestGetMinTolerationTime/with_noExecuteTaints_and_usedTolerations
59.999896389s=== RUN   TestGetMinTolerationTime/usedTolerantion.TolerationSeconds_is_nil
-1ns=== RUN   TestGetMinTolerationTime/noExecuteTaints.TimeAdded_is_nil
-1ns=== RUN   TestGetMinTolerationTime/find_the_latest_trigger_time
59.999839202s=== RUN   TestGetMinTolerationTime/trigger_time_is_up
0s--- PASS: TestGetMinTolerationTime (0.00s)
    --- PASS: TestGetMinTolerationTime/no_noExecuteTaints (0.00s)
    --- PASS: TestGetMinTolerationTime/no_usedTolerations (0.00s)
    --- PASS: TestGetMinTolerationTime/with_noExecuteTaints_and_usedTolerations (0.00s)
    --- PASS: TestGetMinTolerationTime/usedTolerantion.TolerationSeconds_is_nil (0.00s)
    --- PASS: TestGetMinTolerationTime/noExecuteTaints.TimeAdded_is_nil (0.00s)
    --- PASS: TestGetMinTolerationTime/find_the_latest_trigger_time (0.00s)
    --- PASS: TestGetMinTolerationTime/trigger_time_is_up (0.00s)
PASS
ok  	github.com/karmada-io/karmada/pkg/util/helper	(cached)

Output of gotestsum, abnormal:

-bash-5.0# gotestsum ./pkg/util/helper -run=GetMinTolerationTime -v
✓  pkg/util/helper (cached)

=== Failed
=== FAIL: pkg/util/helper TestGetMinTolerationTime (unknown)
-1ns=== RUN   TestGetMinTolerationTime/no_usedTolerations
0s=== RUN   TestGetMinTolerationTime/with_noExecuteTaints_and_usedTolerations
59.999439296s=== RUN   TestGetMinTolerationTime/usedTolerantion.TolerationSeconds_is_nil
-1ns=== RUN   TestGetMinTolerationTime/noExecuteTaints.TimeAdded_is_nil
-1ns=== RUN   TestGetMinTolerationTime/find_the_latest_trigger_time
59.999302798s=== RUN   TestGetMinTolerationTime/trigger_time_is_up
0s--- PASS: TestGetMinTolerationTime (0.00s)
    --- PASS: TestGetMinTolerationTime/no_noExecuteTaints (0.00s)
    --- PASS: TestGetMinTolerationTime/no_usedTolerations (0.00s)
    --- PASS: TestGetMinTolerationTime/with_noExecuteTaints_and_usedTolerations (0.00s)
    --- PASS: TestGetMinTolerationTime/usedTolerantion.TolerationSeconds_is_nil (0.00s)
    --- PASS: TestGetMinTolerationTime/noExecuteTaints.TimeAdded_is_nil (0.00s)
    --- PASS: TestGetMinTolerationTime/find_the_latest_trigger_time (0.00s)
    --- PASS: TestGetMinTolerationTime/trigger_time_is_up (0.00s)

=== FAIL: pkg/util/helper TestGetMinTolerationTime/no_noExecuteTaints (unknown)
-1ns=== RUN   TestGetMinTolerationTime/no_usedTolerations
0s=== RUN   TestGetMinTolerationTime/with_noExecuteTaints_and_usedTolerations
59.999439296s=== RUN   TestGetMinTolerationTime/usedTolerantion.TolerationSeconds_is_nil
-1ns=== RUN   TestGetMinTolerationTime/noExecuteTaints.TimeAdded_is_nil
-1ns=== RUN   TestGetMinTolerationTime/find_the_latest_trigger_time
59.999302798s=== RUN   TestGetMinTolerationTime/trigger_time_is_up
0s--- PASS: TestGetMinTolerationTime (0.00s)
    --- PASS: TestGetMinTolerationTime/no_noExecuteTaints (0.00s)
    --- PASS: TestGetMinTolerationTime/no_usedTolerations (0.00s)
    --- PASS: TestGetMinTolerationTime/with_noExecuteTaints_and_usedTolerations (0.00s)
    --- PASS: TestGetMinTolerationTime/usedTolerantion.TolerationSeconds_is_nil (0.00s)
    --- PASS: TestGetMinTolerationTime/noExecuteTaints.TimeAdded_is_nil (0.00s)
    --- PASS: TestGetMinTolerationTime/find_the_latest_trigger_time (0.00s)
    --- PASS: TestGetMinTolerationTime/trigger_time_is_up (0.00s)

DONE 2 tests, 2 failures in 1.006s

The gotestsum shows 2 failures but returns 0.

DONE 2 tests, 2 failures in 1.006s
-bash-5.0# echo $?
0

Do you know why gotestsum act like this?

@karmada-bot karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 20, 2024
@khanhtc1202
Copy link
Contributor Author

khanhtc1202 commented Apr 20, 2024

@RainbowMango
I found the reason and fixed it by this PR: #4852
We have a print statement in the test logic, and it causes this output in the test JSON output

JSON output

< {"Time":"2024-04-19T23:11:03.422127+07:00","Action":"output","Package":"command-line-arguments","Test":"TestGetMinTolerationTime/no_noExecuteTaints","Output":"=== RUN   TestGetMinTolerationTime/no_noExecuteTaints\n"}
< {"Time":"2024-04-19T23:11:03.422137+07:00","Action":"output","Package":"command-line-arguments","Test":"TestGetMinTolerationTime/no_noExecuteTaints","Output":"-1ns"}
< {"Time":"2024-04-19T23:11:03.422154+07:00","Action":"output","Package":"command-line-arguments","Test":"TestGetMinTolerationTime/no_noExecuteTaints","Output":"--- PASS: TestGetMinTolerationTime/no_noExecuteTaints (0.00s)\n"}

That -1ns makes gotestsum can not parse the go test output correctly 😄 (Contacted the gotestsum maintainer team and got the answer here gotestyourself/gotestsum#400).

I think we can remove that printout statement since printing in the test is just for debugging, and it doesn't have any meaning. With that PR being merged, this change would be fine.

@RainbowMango
Copy link
Member

Good Job!
Shall we rebase this PR since #4852 already merged?

@karmada-bot karmada-bot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Apr 20, 2024
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karmada-bot karmada-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 20, 2024
@khanhtc1202
Copy link
Contributor Author

Something went wrong with my branch commit, let me open another one

@khanhtc1202 khanhtc1202 mentioned this pull request Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants