Fix TestReconcileBatchJob #2350

forsaken628 · 2024-06-10T17:45:30Z

What this PR does / why we need it:
Fix unstable tests
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
#1649

Checklist:

Docs included if any changes are user facing

Signed-off-by: forsaken628 <forsaken628@gmail.com>

tenzen-y · 2024-06-12T10:14:38Z

@forsaken628 Thank you for this contribution.
Could you explain the reason why the flakiness happened and what you fixed the root cause in the PR description?

forsaken628 · 2024-06-12T10:50:51Z

According to the previous logic，call GetTrialObservationLog will return observationLogAvailable once and then observationLogUnavailable multiple times.

I investigated the cause of the failure in the ci environment: in test 2, which should have returned observationLogAvailable, it actually returned observationLogUnavailable, indicating that GetTrialObservationLog was accidentally called once somewhere.

Instead of locating where the accident occurred, I rewrote mockManagerClient to always return the same reply before calling DeleteTrialObservationLog, which I think is consistent with the semantics of ManagerClient.

tenzen-y · 2024-06-13T18:00:08Z

According to the previous logic，call GetTrialObservationLog will return observationLogAvailable once and then observationLogUnavailable multiple times.

I investigated the cause of the failure in the ci environment: in test 2, which should have returned observationLogAvailable, it actually returned observationLogUnavailable, indicating that GetTrialObservationLog was accidentally called once somewhere.

Instead of locating where the accident occurred, I rewrote mockManagerClient to always return the same reply before calling DeleteTrialObservationLog, which I think is consistent with the semantics of ManagerClient.

Thank you for clarifying the root cause. It seems that you split out multiple test case into dedicated case based on the result of the GetTrialObservationLog. That sounds reasonable.

tenzen-y · 2024-06-13T18:03:41Z

pkg/controller.v1beta1/trial/trial_controller_test.go

TBH, we should reimplement these tests as ginkgo BDD style, but that is out of scope of this PR.
So, I leave such refactoring in the future.

tenzen-y · 2024-06-13T18:13:34Z

pkg/controller.v1beta1/trial/trial_controller_test.go

-	mockManagerClient.EXPECT().DeleteTrialObservationLog(gomock.Any()).Return(nil, nil).AnyTimes()
+	t.Run(`Trial run with "Failed" BatchJob.`, func(t *testing.T) {
+		g := gomega.NewGomegaWithT(t)
+		mockMC.msg = observationLogUnavailable


Suggested change

mockMC.msg = observationLogUnavailable

mockManagerClient.EXPECT().GetTrialObservationLog(gomock.Any()).Return(observationLogAvailable, nil).AnyTimes()

Instead of this original mock client, can we reuse the autogenerated mock clients like this?

We need WithOverridableExpectations here, but github.com/golang/mock/mock does not support this feature.
Or "Each test should create a new Controller".
In my opinion, gomock is not flexible enough, and it complicates a simple thing for this scenario.

Finally, I found another way.

@forsaken628 I opened the issue to replace gomock with go.uber.com/gomock here: #2356

As a separate PR, could you work on the issue?

pkg/controller.v1beta1/trial/trial_controller_test.go

Signed-off-by: forsaken628 <forsaken628@gmail.com>

tenzen-y

/lgtm
/approve

Thank you for the great contribution!

google-oss-prow · 2024-06-14T06:40:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y

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:

~~OWNERS~~ [tenzen-y]

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

andreyvelich · 2024-06-14T13:42:52Z

Thank you for this amazing contribution @forsaken628!
I really hope it will improve our unstable tests.

* update Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * cleanup Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * use gomock Signed-off-by: forsaken628 <forsaken628@gmail.com> --------- Signed-off-by: forsaken628 <forsaken628@gmail.com>

* update Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * cleanup Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * use gomock Signed-off-by: forsaken628 <forsaken628@gmail.com> --------- Signed-off-by: forsaken628 <forsaken628@gmail.com> Signed-off-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>

…branch (#2362) * Fix TestReconcileBatchJob (#2350) * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * cleanup Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * use gomock Signed-off-by: forsaken628 <forsaken628@gmail.com> --------- Signed-off-by: forsaken628 <forsaken628@gmail.com> Signed-off-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> * Use cache-dependency-path in actions/setup-go for CI workflow (#2355) Signed-off-by: forsaken628 <forsaken628@gmail.com> Signed-off-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> * Replace already closed github.com/golang/mock with go.uber.org/mock (#2357) * replace gomock Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * revert Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> --------- Signed-off-by: forsaken628 <forsaken628@gmail.com> Signed-off-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> * Replace gRPC code generation tool from Znly/protoc to Buf (#2344) * Replace gRPC code generation tool from Znly/protoc to Buf Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * del build.sh Signed-off-by: forsaken628 <forsaken628@gmail.com> * cleanup Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix test Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * refine Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * rm outter yaml Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> --------- Signed-off-by: forsaken628 <forsaken628@gmail.com> Signed-off-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> * Upgrade the protobuf version to >=4.21.12,<5 (#2358) Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com> Signed-off-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> * [SDK] Fix empty list for env variables and numpy version (#2360) * [SDK] Fix empty list for env variables Signed-off-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> * Fix numpy version in tests Signed-off-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> --------- Signed-off-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> --------- Signed-off-by: forsaken628 <forsaken628@gmail.com> Signed-off-by: Andrey Velichkevich <andrey.velichkevich@gmail.com> Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com> Co-authored-by: coldWater <forsaken628@gmail.com> Co-authored-by: Yuki Iwai <yuki.iwai.tz@gmail.com>

* update Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * cleanup Signed-off-by: forsaken628 <forsaken628@gmail.com> * fix Signed-off-by: forsaken628 <forsaken628@gmail.com> * update Signed-off-by: forsaken628 <forsaken628@gmail.com> * use gomock Signed-off-by: forsaken628 <forsaken628@gmail.com> --------- Signed-off-by: forsaken628 <forsaken628@gmail.com>

forsaken628 added 8 commits June 10, 2024 22:28

update

2e204c1

Signed-off-by: forsaken628 <forsaken628@gmail.com>

fix

13e39b3

Signed-off-by: forsaken628 <forsaken628@gmail.com>

update

6b6e249

Signed-off-by: forsaken628 <forsaken628@gmail.com>

update

ac0c04e

Signed-off-by: forsaken628 <forsaken628@gmail.com>

update

539ca6e

Signed-off-by: forsaken628 <forsaken628@gmail.com>

fix

f0d5fd7

Signed-off-by: forsaken628 <forsaken628@gmail.com>

cleanup

4a52a18

Signed-off-by: forsaken628 <forsaken628@gmail.com>

fix

b4caa68

Signed-off-by: forsaken628 <forsaken628@gmail.com>

google-oss-prow bot requested review from andreyvelich, anencore94 and gaocegege June 10, 2024 17:45

google-oss-prow bot added the size/L label Jun 10, 2024

update

5cb9b2b

Signed-off-by: forsaken628 <forsaken628@gmail.com>

tenzen-y reviewed Jun 13, 2024

View reviewed changes

use gomock

317c40d

Signed-off-by: forsaken628 <forsaken628@gmail.com>

tenzen-y mentioned this pull request Jun 14, 2024

Replace already closed github.com/golang/mock with go.uber.org/mock #2356

Closed

tenzen-y reviewed Jun 14, 2024

View reviewed changes

google-oss-prow bot assigned tenzen-y Jun 14, 2024

google-oss-prow bot added the lgtm label Jun 14, 2024

google-oss-prow bot added the approved label Jun 14, 2024

google-oss-prow bot merged commit 4e4ce6f into kubeflow:master Jun 14, 2024
60 checks passed

This was referenced Jun 14, 2024

Go Test failed sometimes #1649

Closed

Flaky Test: Trial status is succeeded and metrics are properly populated #2269

Closed

Replace already closed github.com/golang/mock with go.uber.org/mock #2357

Merged

forsaken628 deleted the test-failed branch June 14, 2024 15:20

andreyvelich mentioned this pull request Jun 18, 2024

Cherry pick of #2350 #2355 #2357 #2344 #2358 #2360 into release-0.17 branch #2362

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TestReconcileBatchJob #2350

Fix TestReconcileBatchJob #2350

forsaken628 commented Jun 10, 2024

tenzen-y commented Jun 12, 2024

forsaken628 commented Jun 12, 2024

tenzen-y commented Jun 13, 2024

tenzen-y Jun 13, 2024 •

edited

Loading

tenzen-y Jun 13, 2024

forsaken628 Jun 14, 2024

forsaken628 Jun 14, 2024

tenzen-y Jun 14, 2024

tenzen-y left a comment

google-oss-prow bot commented Jun 14, 2024

andreyvelich commented Jun 14, 2024

	mockMC.msg = observationLogUnavailable
	mockManagerClient.EXPECT().GetTrialObservationLog(gomock.Any()).Return(observationLogAvailable, nil).AnyTimes()

Fix TestReconcileBatchJob #2350

Fix TestReconcileBatchJob #2350

Conversation

forsaken628 commented Jun 10, 2024

tenzen-y commented Jun 12, 2024

forsaken628 commented Jun 12, 2024

tenzen-y commented Jun 13, 2024

tenzen-y Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

tenzen-y Jun 13, 2024

Choose a reason for hiding this comment

forsaken628 Jun 14, 2024

Choose a reason for hiding this comment

forsaken628 Jun 14, 2024

Choose a reason for hiding this comment

tenzen-y Jun 14, 2024

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Jun 14, 2024

andreyvelich commented Jun 14, 2024

tenzen-y Jun 13, 2024 •

edited

Loading