Expect exit code enhancement #14672

tjungblu · 2022-11-01T12:05:40Z

ExpectProcess and ExpectFunc now take the exit code of the process into account, not just the matching of the tty output.

Signed-off-by: Thomas Jungblut tjungblu@redhat.com

I'm still fixing some of the tests that are now failing due to this.

serathius · 2022-11-01T15:11:42Z

Nice, one additional option to consider is returning process status so the caller can decide based on exit code. Example linearizability tests expect process to fail when failpoint is injected.

tjungblu · 2022-11-01T17:39:39Z

added the status code as well, I think this is going to become a bigger refactor since we there was no exit code assumption beforehand. Some tests still don't run, I also think there's some locking issue. Gotta continue tomorrow.

tests/e2e/ctl_v3_auth_test.go

ahrtr · 2022-11-02T07:05:16Z

Thanks for working on this.

It seems the PR caused lots of workflow failures. Usually 1~2 failures might be caused by flaky test. But 6 failures most likely mean that it's caused by the PR. Please take a look at the failures.

tjungblu · 2022-11-02T09:43:00Z

Yeah absolutely, this is caused by several commands now failing the expect due to the exit codes.

pkg/expect/expect.go

tjungblu · 2022-11-03T13:19:25Z

@ahrtr @serathius finally managed to get all to green. There are a couple of weird choices I've made around the error assertions, so let me know what you think about it.

serathius · 2022-11-03T13:40:32Z

pkg/expect/expect.go

-		if l != "" {
-			if printDebugLines {
-				fmt.Printf("%s (%s) (%d): %s", ep.cmd.Path, ep.cfg.name, ep.cmd.Process.Pid, l)
+		err := func() error {


no need for anonymous function

disagree, this makes the locking more obvious in keeping the unlock scoped to the block.

ep.mu.Lock() defer ep.mu.Unlock()

I think this could be another func however, if that's a more viable approach for you?

Sounds good.

tests/e2e/ctl_v3_auth_test.go

serathius · 2022-11-03T13:43:30Z

pkg/expect/expect.go

+		return ep.exitCode
+	}
+
+	return math.MinInt


this is weird decision. would prefer to return an error or some known error code like 255

agreed, will change this

serathius · 2022-11-03T13:49:16Z

tests/common/defrag_test.go

-			t.Fatalf("defrag_test: defrag error (%v)", err)
+		err = cc.Defragment(ctx, options)
+		if err != nil {
+			require.ErrorContains(t, err, "Finished defragmenting etcd")


Why defragment command fails here?

odd, works again. Reverted that.

serathius · 2022-11-03T13:50:33Z

tests/e2e/cluster_downgrade_test.go

@@ -47,13 +48,19 @@ func testDowngradeUpgrade(t *testing.T, clusterSize int) {
 		t.Skipf("%q does not exist", lastReleaseBinary)
 	}

-	currentVersion := semver.New(version.Version)
-	lastVersion := semver.Version{Major: currentVersion.Major, Minor: currentVersion.Minor - 1}
+	currentVersion, err := getVersionFromBinary(currentEtcdBinary)


This looks like unrelated changes from other PR. Please rebase.

I can split this into another commit, but it was never passing for me locally. Not sure if that works on your box.

I expect that after rebase those changes will not be needed/present.

I don't understand, the version inconsistency I run into still exists in main:
https://github.com/etcd-io/etcd/blob/main/tests/e2e/cluster_downgrade_test.go#L50-L55

The issue stems from the fact that the logic takes what is assumed to be in the binary, not what is actually reported by it. If you check the diff, it's already against the latest revision of the test.

Can you create a separate PR to fix the downgrade test?

sure, I'll pull this out of that PR.

created it: #14710

serathius · 2022-11-07T09:33:59Z

Great work overall. I expect this will be crucial to reduce flakiness of e2e tests. One think I would like to fix before we merge is separation between Wait and Close. They should behave like their process level counterparts.

pkg/expect/expect.go

serathius

LGTM, this is a big change so lets get also feedback from other maintainers.
cc @ahrtr @spzala @ptabor

tjungblu · 2022-11-09T14:12:16Z

Thanks Marek, I'm still fixing the remainder of the tests that are failing due to the changes

tjungblu · 2022-11-10T09:58:35Z

oh wow, almost all green. The last check seems to be an unrelated flake:

2022-11-09T18:00:33.8660828Z logger.go:130: 2022-11-09T17:57:58.788Z ERROR m0 failed to update storage version {"member": "m0", "cluster-version": "3.6.0", "error": "cannot detect storage schema version: missing confstate information"}

I'm just testing whether the e2e test got a lot longer than before, otherwise this is good to go from my side.

serathius · 2022-11-10T10:24:27Z

oh wow, almost all green.

Great job!

The last check seems to be an unrelated flake:

Rerun the failed scenario

I'm just testing whether the e2e test got a lot longer than before, otherwise this is good to go from my side.

Correctness here is more important then time. Let's revisit it when time becomes a problem.

tjungblu · 2022-11-10T10:43:06Z

Thanks for the rerun. It's almost the same runtime on e2e on my local box. I'll keep an eye on the execution times on the workflow over the next couple days.

ahrtr · 2022-11-11T21:11:22Z

pkg/expect/expect.go

+	if err != nil {
+		return err
+	}
+
+	return nil


Suggested change

if err != nil {

return err

}

return nil

return err

ahrtr · 2022-11-11T21:22:14Z

tests/framework/e2e/cluster.go

-		return fmt.Errorf("failed to remove member: %w", err)
+
+	if !memberRemoved {
+		return fmt.Errorf("failed to remove member after 10 tries")


Minor comment: if there is no parameter, then you'd better use errors.New instead.

ahrtr · 2022-11-14T10:15:39Z

This PR is based on an old commit tjungblu@0d4a516, please rebase this PR, then I will take another look.

ExpectProcess and ExpectFunc now take the exit code of the process into account, not just the matching of the tty output. This also refactors the many tests that were previously succeeding on matching an output from a failing cmd execution. Signed-off-by: Thomas Jungblut <tjungblu@redhat.com>

tjungblu · 2022-11-14T10:20:04Z

@ahrtr just rebased, PTAL.

ahrtr

Overall looks good to me.

Great work. Thank you @tjungblu

tjungblu · 2022-11-14T11:30:43Z

Thanks @ahrtr. Please ping me on any flakes and perf regressions, I'll monitor this throughout the week.

tjungblu mentioned this pull request Nov 1, 2022

[test] Enhance (*ExpectProcess) ExpectFunc #14638

Closed

tjungblu force-pushed the etcd-14638 branch from 72c50e0 to ba3d083 Compare November 1, 2022 14:52

tjungblu force-pushed the etcd-14638 branch from ba3d083 to 549b0a8 Compare November 1, 2022 17:38

tjungblu commented Nov 1, 2022

View reviewed changes

tests/e2e/ctl_v3_auth_test.go Outdated Show resolved Hide resolved

tjungblu force-pushed the etcd-14638 branch 10 times, most recently from fae5399 to e564641 Compare November 2, 2022 16:37

tjungblu commented Nov 2, 2022

View reviewed changes

pkg/expect/expect.go Outdated Show resolved Hide resolved

tjungblu force-pushed the etcd-14638 branch 5 times, most recently from b7e3021 to 5289e73 Compare November 3, 2022 12:48

serathius reviewed Nov 3, 2022

View reviewed changes

tjungblu commented Nov 3, 2022

View reviewed changes

tests/e2e/ctl_v3_auth_test.go Outdated Show resolved Hide resolved

serathius reviewed Nov 3, 2022

View reviewed changes

tjungblu force-pushed the etcd-14638 branch 2 times, most recently from bb5724c to ea584b2 Compare November 9, 2022 12:51

serathius reviewed Nov 9, 2022

View reviewed changes

pkg/expect/expect.go Outdated Show resolved Hide resolved

serathius approved these changes Nov 9, 2022

View reviewed changes

tjungblu force-pushed the etcd-14638 branch 4 times, most recently from 82f22b4 to 38ff2ce Compare November 9, 2022 17:51

ahrtr self-requested a review November 10, 2022 00:49

ahrtr reviewed Nov 11, 2022

View reviewed changes

tjungblu force-pushed the etcd-14638 branch from 38ff2ce to 972c2b0 Compare November 14, 2022 09:17

tjungblu requested a review from ahrtr November 14, 2022 09:18

tjungblu force-pushed the etcd-14638 branch from 972c2b0 to 5b163aa Compare November 14, 2022 10:20

ahrtr approved these changes Nov 14, 2022

View reviewed changes

ahrtr added the stage/merge-when-tests-green label Nov 14, 2022

serathius merged commit 4cdcb91 into etcd-io:main Nov 14, 2022

This was referenced Nov 15, 2022

TestKVDelete flake #14762

Closed

TestCtlV3Lock flake #14763

Closed

mkumatag mentioned this pull request Nov 24, 2022

TestSignal unit test failes #14846

Closed

serathius mentioned this pull request Oct 7, 2023

Inject sleep during etcd bootstrap to reproduce etcd-io#16666 #16691

Merged

fuweid mentioned this pull request Dec 20, 2023

[3.5] member replace e2e test #17123

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expect exit code enhancement #14672

Expect exit code enhancement #14672

tjungblu commented Nov 1, 2022 •

edited

Loading

serathius commented Nov 1, 2022 •

edited

Loading

tjungblu commented Nov 1, 2022

ahrtr commented Nov 2, 2022

tjungblu commented Nov 2, 2022

tjungblu commented Nov 3, 2022

serathius Nov 3, 2022

tjungblu Nov 3, 2022 •

edited

Loading

serathius Nov 7, 2022

serathius Nov 3, 2022

tjungblu Nov 3, 2022

serathius Nov 3, 2022

tjungblu Nov 3, 2022

serathius Nov 3, 2022

tjungblu Nov 3, 2022

serathius Nov 7, 2022

tjungblu Nov 7, 2022

serathius Nov 8, 2022

tjungblu Nov 8, 2022

tjungblu Nov 8, 2022

serathius commented Nov 7, 2022

serathius left a comment

tjungblu commented Nov 9, 2022

tjungblu commented Nov 10, 2022

serathius commented Nov 10, 2022

tjungblu commented Nov 10, 2022

ahrtr Nov 11, 2022

tjungblu Nov 14, 2022

ahrtr Nov 11, 2022

tjungblu Nov 14, 2022

ahrtr commented Nov 14, 2022

tjungblu commented Nov 14, 2022

ahrtr left a comment

tjungblu commented Nov 14, 2022

Expect exit code enhancement #14672

Expect exit code enhancement #14672

Conversation

tjungblu commented Nov 1, 2022 • edited Loading

serathius commented Nov 1, 2022 • edited Loading

tjungblu commented Nov 1, 2022

ahrtr commented Nov 2, 2022

tjungblu commented Nov 2, 2022

tjungblu commented Nov 3, 2022

Choose a reason for hiding this comment

tjungblu Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serathius commented Nov 7, 2022

serathius left a comment

Choose a reason for hiding this comment

tjungblu commented Nov 9, 2022

tjungblu commented Nov 10, 2022

serathius commented Nov 10, 2022

tjungblu commented Nov 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrtr commented Nov 14, 2022

tjungblu commented Nov 14, 2022

ahrtr left a comment

Choose a reason for hiding this comment

tjungblu commented Nov 14, 2022

tjungblu commented Nov 1, 2022 •

edited

Loading

serathius commented Nov 1, 2022 •

edited

Loading

tjungblu Nov 3, 2022 •

edited

Loading