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.

Sign up for GitHub

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

Jump to bottom

do not start new tests after the first test failure #2082

Merged
merged 1 commit into from
Jun 29, 2022
Merged

do not start new tests after the first test failure #2082

merged 1 commit into from
Jun 29, 2022

Conversation

my-git9
Copy link
Member

@my-git9 my-git9 commented Jun 29, 2022

What type of PR is this?
Do not start new tests after the first test failure

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes # #2073

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

@karmada-bot karmada-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. labels Jun 29, 2022
@RainbowMango
Copy link
Member

Could you please help to test if this flag works as expected? I guess you can hack one of the unit tests to make a failing test.

@my-git9
Copy link
Member Author

my-git9 commented Jun 29, 2022

Could you please help to test if this flag works as expected? I guess you can hack one of the unit tests to make a failing test.

I change cmd/agent/app/options/validation_test.go
image
to
image

And test:

$  go test --race -v ./cmd/...
?       github.com/karmada-io/karmada/cmd/agent [no test files]
?       github.com/karmada-io/karmada/cmd/agent/app     [no test files]
=== RUN   TestValidateKarmadaAgentConfiguration
    validation_test.go:44: expected success: []
    validation_test.go:44: expected success: []
--- FAIL: TestValidateKarmadaAgentConfiguration (0.00s)
FAIL
FAIL    github.com/karmada-io/karmada/cmd/agent/app/options     0.558s
?       github.com/karmada-io/karmada/cmd/aggregated-apiserver  [no test files]
?       github.com/karmada-io/karmada/cmd/aggregated-apiserver/app      [no test files]
?       github.com/karmada-io/karmada/cmd/aggregated-apiserver/app/options      [no test files]
?       github.com/karmada-io/karmada/cmd/controller-manager    [no test files]
?       github.com/karmada-io/karmada/cmd/controller-manager/app        [no test files]
=== RUN   TestValidateControllerManagerConfiguration
--- PASS: TestValidateControllerManagerConfiguration (0.00s)
PASS
ok      github.com/karmada-io/karmada/cmd/controller-manager/app/options        (cached)
?       github.com/karmada-io/karmada/cmd/descheduler   [no test files]
?       github.com/karmada-io/karmada/cmd/descheduler/app       [no test files]
=== RUN   TestValidateKarmadaDescheduler
--- PASS: TestValidateKarmadaDescheduler (0.00s)
PASS
ok      github.com/karmada-io/karmada/cmd/descheduler/app/options       (cached)
?       github.com/karmada-io/karmada/cmd/karmada-search        [no test files]
?       github.com/karmada-io/karmada/cmd/karmada-search/app    [no test files]
?       github.com/karmada-io/karmada/cmd/karmada-search/app/options    [no test files]
?       github.com/karmada-io/karmada/cmd/karmadactl    [no test files]
?       github.com/karmada-io/karmada/cmd/kubectl-karmada       [no test files]
?       github.com/karmada-io/karmada/cmd/scheduler     [no test files]
?       github.com/karmada-io/karmada/cmd/scheduler/app [no test files]
=== RUN   TestValidateKarmadaSchedulerConfiguration
--- PASS: TestValidateKarmadaSchedulerConfiguration (0.00s)
PASS
ok      github.com/karmada-io/karmada/cmd/scheduler/app/options (cached)
?       github.com/karmada-io/karmada/cmd/scheduler-estimator   [no test files]
?       github.com/karmada-io/karmada/cmd/scheduler-estimator/app       [no test files]
=== RUN   TestValidateKarmadaSchedulerEstimator
--- PASS: TestValidateKarmadaSchedulerEstimator (0.00s)
PASS
ok      github.com/karmada-io/karmada/cmd/scheduler-estimator/app/options       (cached)
?       github.com/karmada-io/karmada/cmd/webhook       [no test files]
?       github.com/karmada-io/karmada/cmd/webhook/app   [no test files]
=== RUN   TestValidateKarmadaWebhookConfiguration
--- PASS: TestValidateKarmadaWebhookConfiguration (0.00s)
PASS
ok      github.com/karmada-io/karmada/cmd/webhook/app/options   (cached)
FAIL

AND

% go test -race -failfast -v ./cmd/...
?       github.com/karmada-io/karmada/cmd/agent [no test files]
?       github.com/karmada-io/karmada/cmd/agent/app     [no test files]
=== RUN   TestValidateKarmadaAgentConfiguration
    validation_test.go:44: expected success: []
    validation_test.go:44: expected success: []
--- FAIL: TestValidateKarmadaAgentConfiguration (0.00s)
FAIL
FAIL    github.com/karmada-io/karmada/cmd/agent/app/options     0.593s
?       github.com/karmada-io/karmada/cmd/aggregated-apiserver  [no test files]
?       github.com/karmada-io/karmada/cmd/aggregated-apiserver/app      [no test files]
?       github.com/karmada-io/karmada/cmd/aggregated-apiserver/app/options      [no test files]
?       github.com/karmada-io/karmada/cmd/controller-manager    [no test files]
?       github.com/karmada-io/karmada/cmd/controller-manager/app        [no test files]
=== RUN   TestValidateControllerManagerConfiguration
--- PASS: TestValidateControllerManagerConfiguration (0.00s)
PASS
ok      github.com/karmada-io/karmada/cmd/controller-manager/app/options        1.432s
?       github.com/karmada-io/karmada/cmd/descheduler   [no test files]
?       github.com/karmada-io/karmada/cmd/descheduler/app       [no test files]
=== RUN   TestValidateKarmadaDescheduler
--- PASS: TestValidateKarmadaDescheduler (0.00s)
PASS
ok      github.com/karmada-io/karmada/cmd/descheduler/app/options       0.957s
?       github.com/karmada-io/karmada/cmd/karmada-search        [no test files]
?       github.com/karmada-io/karmada/cmd/karmada-search/app    [no test files]
?       github.com/karmada-io/karmada/cmd/karmada-search/app/options    [no test files]
?       github.com/karmada-io/karmada/cmd/karmadactl    [no test files]
?       github.com/karmada-io/karmada/cmd/kubectl-karmada       [no test files]
?       github.com/karmada-io/karmada/cmd/scheduler     [no test files]
?       github.com/karmada-io/karmada/cmd/scheduler/app [no test files]
=== RUN   TestValidateKarmadaSchedulerConfiguration
--- PASS: TestValidateKarmadaSchedulerConfiguration (0.00s)
PASS
ok      github.com/karmada-io/karmada/cmd/scheduler/app/options 1.669s
?       github.com/karmada-io/karmada/cmd/scheduler-estimator   [no test files]
?       github.com/karmada-io/karmada/cmd/scheduler-estimator/app       [no test files]
=== RUN   TestValidateKarmadaSchedulerEstimator
--- PASS: TestValidateKarmadaSchedulerEstimator (0.00s)
PASS
ok      github.com/karmada-io/karmada/cmd/scheduler-estimator/app/options       0.192s
?       github.com/karmada-io/karmada/cmd/webhook       [no test files]
?       github.com/karmada-io/karmada/cmd/webhook/app   [no test files]
=== RUN   TestValidateKarmadaWebhookConfiguration
--- PASS: TestValidateKarmadaWebhookConfiguration (0.00s)
PASS
ok      github.com/karmada-io/karmada/cmd/webhook/app/options   0.237s
FAIL

It seems that there is no effect, is my test method wrong?

@my-git9
Copy link
Member Author

my-git9 commented Jun 29, 2022

The new go test -failfast flag disables running additional tests after any test fails. Note that tests running in parallel with the failing test are allowed to complete.

@my-git9
Copy link
Member Author

my-git9 commented Jun 29, 2022

A issue about this: golang/go#33038

@XiShanYongYe-Chang
Copy link
Member

A issue about this: golang/go#33038

This issue looks like hang on for a long time. How about update with this: golang/go#33038 (comment)

@my-git9
Copy link
Member Author

my-git9 commented Jun 29, 2022

A issue about this: golang/go#33038

This issue looks like hang on for a long time. How about update with this: golang/go#33038 (comment)

LGTM. /cc @RainbowMango

@my-git9
Copy link
Member Author

my-git9 commented Jun 29, 2022

For that:
image

Test result:

% for s in $(go list ./cmd/...); do if ! go test -race -failfast -v -p 1 $s; then break; fi; done
?       github.com/karmada-io/karmada/cmd/agent [no test files]
?       github.com/karmada-io/karmada/cmd/agent/app     [no test files]
=== RUN   TestValidateKarmadaAgentConfiguration
    validation_test.go:44: expected success: []
    validation_test.go:44: expected success: []
--- FAIL: TestValidateKarmadaAgentConfiguration (0.00s)
FAIL
FAIL    github.com/karmada-io/karmada/cmd/agent/app/options     0.751s
FAIL

Seems good

@RainbowMango
Copy link
Member

Great!
/lgtm
/approve

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2022
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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

The pull request process is described 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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2022
@karmada-bot karmada-bot merged commit cee1a2f into karmada-io:master Jun 29, 2022
@ikaven1024
Copy link
Member

Hi, @my-git9 @RainbowMango UT seems not work now, please take a look.
https://github.com/karmada-io/karmada/runs/7147189177?check_suite_focus=true#step:4:1

@ikaven1024
Copy link
Member

In makefile, you shall use $$ for a shell variable, test it below:

diff --git a/Makefile b/Makefile
index 9a5aed43..929a45e3 100644
--- a/Makefile
+++ b/Makefile
@@ -110,9 +110,9 @@ release-chart:
 
 .PHONY: test
 test:
-       for s in $(go list ./pkg/...); do if ! go test -race -failfast -v -p 1 $s; then break; fi; done
-       for s in $(go list ./cmd/...); do if ! go test -race -failfast -v -p 1 $s; then break; fi; done
-       for s in $(go list ./examples/...); do if ! go test -race -failfast -v -p 1 $s; then break; fi; done
+       for s in $$(go list ./pkg/...); do if ! go test -race -failfast -v -p 1 $${s}; then break; fi; done
+       for s in $$(go list ./cmd/...); do if ! go test -race -failfast -v -p 1 $${s}; then break; fi; done
+       for s in $$(go list ./examples/...); do if ! go test -race -failfast -v -p 1 $${s}; then break; fi; done

@ikaven1024
Copy link
Member

ikaven1024 commented Jul 2, 2022 •
edited
Loading

But i am wonder, is it beneficial introducing failfast, abandoning parallel test？

@RainbowMango
Copy link
Member

But i am wonder, is it beneficial introducing failfast, abandoning parallel test？

Why do you think -failfast would abandon the parallel tests?
As far as I know, we don't have parallel tests yet, but I think we shouldn't abandon them.

@ikaven1024
Copy link
Member

go test runs in parallel tests for different packages. But in this scrip, each go test in for statement only runs one package. So all the tests are sequential.

https://stackoverflow.com/questions/24375966/does-go-test-run-unit-tests-concurrently#:~:text=by%20default%20the%20command%20go%20test%20runs%20in%20parallel%20tests%20for%20different%20packages

@ikaven1024
Copy link
Member 

test:
	go clean -testcache
	go test --race --v ./pkg/...
	go test --race --v ./cmd/...
	go test --race --v ./examples/...

real    0m20.817s
user    0m48.761s
sys     0m12.330s
test:
	go clean -testcache
	for s in $$(go list ./pkg/...); do if ! go test -race -failfast -v -p 1 $${s}; then break; fi; done
	for s in $$(go list ./cmd/...); do if ! go test -race -failfast -v -p 1 $${s}; then break; fi; done
	for s in $$(go list ./examples/...); do if ! go test -race -failfast -v -p 1 $${s}; then break; fi; done

real    2m53.020s
user    2m6.859s
sys     1m51.134s

@RainbowMango
Copy link
Member

Got it. Thanks for the explanation.

But I found it's pretty inconvenient to get the failed case without -failfast.
Any idea ?

Do you think we should revert the change made by this PR?

@RainbowMango
Copy link
Member

I'm thinking if we can introduce two endpoints in Makefile for different purposes, one is for developers run test locally and the other one for CI(I assume CI is time-insensitive).

@RainbowMango
Copy link
Member

Talked with @ikaven1024, and given the UT is totally missing, for now, we are going to revert this change immediately.

@RainbowMango RainbowMango mentioned this pull request Jul 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Garrybest Garrybest Awaiting requested review from Garrybest

@XiShanYongYe-Chang XiShanYongYe-Chang Awaiting requested review from XiShanYongYe-Chang

Assignees

@RainbowMango RainbowMango

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@my-git9 @RainbowMango @XiShanYongYe-Chang @karmada-bot @ikaven1024