e2e: redefine ExitWithError() to require exit code #22270

edsantiago · 2024-04-05T01:14:10Z

...and an optional error-message string, to be checked against stderr.

This is a starting point and baby-steps progress toward #18188. There are 249 ExitWithError() checks in test/e2e. It will take weeks to fix them all. This commit enables new functionality:

Expect(ExitWithError(125, "expected substring"))

...while also allowing the current empty-args form. Once all 249 empty-args uses are modernized, the matcher code will be cleaned up.

I expect it will take several months of light effort to get all e2e tests transitioned to the new form. I am choosing to do so in pieces, for (relative) ease of review. This PR:

makes the initial changes described above; and
updates a small subset of e2e _test.go files such that:
a) ExitWithError() is given an exit code and error string; and
b) Exit(Nonzero) is changed to ExitWithError(Nonzero, "string") (when possible)

None

openshift-ci · 2024-04-05T01:14:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago

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~~ [edsantiago]

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

Luap99

I think conceptionally this is fine. We can then extend it to check that stderr is always checked, i.e. if no message is given to ExitWithError() then stderr must be empty which is needed for command like podman run alpine false

As far as remote vs local goes, we can always do
IsRemote{ matcher1 } else { matcher2 } where it is actually required.

Luap99 · 2024-04-05T09:55:42Z

test/e2e/checkpoint_image_test.go

@@ -35,7 +35,7 @@ var _ = Describe("Podman checkpoint", func() {
 		checkpointImage := "foobar-checkpoint"
 		session := podmanTest.Podman([]string{"container", "checkpoint", "--create-image", checkpointImage, "foobar"})
 		session.WaitWithDefaultTimeout()
-		Expect(session).To(ExitWithError())
+		Expect(session).To(ExitWithError(125, "no container with name or ID \"foobar\" found: no such container"))
 		Expect(session.ErrorToString()).To(ContainSubstring("no container with name or ID \"foobar\" found"))


the line already checks the stderr

Luap99 · 2024-04-05T09:57:09Z

test/utils/matchers.go

+	// FIXME: once all ExitWithError()s have been migrated to new form,
+	//        change interface to (int, ...string)


+1 that is important for proper type hints and compile time enforcement

mheon · 2024-04-08T19:48:18Z

I like this. LGTM so far.

edsantiago · 2024-04-08T19:51:30Z

Thanks. I'm slowly replacing ExitWithError instances in my work branch and am debating submitting what I've got (for ease of review) or continuing with a monster PR. Any preferences?

edsantiago

Pointing out a couple of instances demonstrating the value of this.

This has grown to just about what I think is reasonable for review. Maybe a little over (sorry). I will pause here, accept feedback, then proceed with the process once this merges.

edsantiago · 2024-04-09T17:28:18Z

test/e2e/exec_test.go

-	It("podman exec without command", func() {
-		session := podmanTest.Podman([]string{"exec", "foobar"})
-		session.WaitWithDefaultTimeout()
-		Expect(session).Should(Exit(125))
+		Expect(session).Should(ExitWithError(125, "no container with name or ID \"foobar\" found: no such container"))


This is why I'm "wasting" my time on this seemingly-trivial exercise.

This entire test was pointless. I'm sure the author wanted it to fail with "must provide a non-empty command", but in reality it was failing with "container foobar does not exist and I'm not even going to bother checking the rest of the args".

I added this test (as intended) as a piggyback onto simple command below.

edsantiago · 2024-04-09T17:32:26Z

test/e2e/generate_systemd_test.go

-	It("podman generate systemd bad restart policy", func() {
-		session := podmanTest.Podman([]string{"generate", "systemd", "--restart-policy", "never", "foobar"})
-		session.WaitWithDefaultTimeout()
-		Expect(session).To(ExitWithError())


Another completely pointless test: this was failing with "I'm gonna check the container name first, before even checking options, and foobar doesn't exist, so this test doesn't even test what you think it's testing". Fortunately this was a dup, and the "bogus" test below was already testing this functionality.

Luap99 · 2024-04-10T11:36:29Z

test/e2e/generate_systemd_test.go

 	})

 	It("podman generate systemd bad timeout value", func() {
 		session := podmanTest.Podman([]string{"generate", "systemd", "--time", "-1", "foobar"})
 		session.WaitWithDefaultTimeout()
-		Expect(session).To(ExitWithError())
+		Expect(session).To(ExitWithError(125, "invalid argument \"-1\" for \"-t, --time\" flag: strconv.ParseUint: parsing \"-1\": invalid syntax"))


hint, if you like to avoid all the quoting you can use backticks to create a "raw" string without any required escaping, i.e.

`invalid argument "-1" for "-t, --time" flag: strconv.ParseUint: parsing "-1": invalid syntax`

Not required of course but IMO makes it easer to read.

Thank you! I prefer the backtick form too, but its use in our code base is rare. I'm in the process of fixing these (only in my changes) and will repush.

test/e2e/exec_test.go

Luap99 · 2024-04-10T11:44:53Z

test/e2e/create_staticip_test.go

@@ -74,7 +73,7 @@ var _ = Describe("Podman create with --ip flag", func() {
 		// test1 container is running with the given IP.
 		result = podmanTest.Podman([]string{"start", "-a", "test2"})
 		result.WaitWithDefaultTimeout()
-		Expect(result).To(ExitWithError())
+		Expect(result).To(ExitWithError(125))


AFAIK we dropped CNI tests so could you just simply this block below and only match the netavark message?

Luap99 · 2024-04-10T11:46:11Z

overall LGTM, very good improvements

edsantiago · 2024-04-10T12:08:48Z

test/e2e/create_staticip_test.go

-		} else if podmanTest.NetworkBackend == Netavark {
-			Expect(result.ErrorToString()).To(ContainSubstring("requested ip address %s is already allocated", ip))
-		}
+		Expect(result).To(ExitWithError(125, " is already allocated"))


@Luap99 is this what you meant?

yes although if we do not want to loose the ip I suggest using

fmt.Sprintf("requested ip address %s is already allocated", ip)

to create the proper error string
But I do not think it is super important one way or another so I am fine with the current version as well.

Fair enough. I think ExitWithError(xxx, fmt.Sprintf())` looks clunky, but tighter testing is better testing. So done.

ContainSubstring() is kind of nice in how it takes varargs. I think that's nicely readable, but I just didn't want to complicate this PR even more by adding that to ExitWithError(). Would that be worth pursuing once this exercise is done and I tighten up the ExitWithError() signature?

ContainSubstring() is kind of nice in how it takes varargs. I think that's nicely readable, but I just didn't want to complicate this PR even more by adding that to ExitWithError(). Would that be worth pursuing once this exercise is done and I tighten up the ExitWithError() signature?

Yes I think that would be valuable but definitely not worth to block this work over it.

...and an optional error-message string, to be checked against stderr. This is a starting point and baby-steps progress toward containers#18188. There are 249 ExitWithError() checks in test/e2e. It will take weeks to fix them all. This commit enables new functionality: Expect(ExitWithError(125, "expected substring")) ...while also allowing the current empty-args form. Once all 249 empty-args uses are modernized, the matcher code will be cleaned up. I expect it will take several months of light effort to get all e2e tests transitioned to the new form. I am choosing to do so in pieces, for (relative) ease of review. This PR: 1) makes the initial changes described above; and 2) updates a small subset of e2e _test.go files such that: a) ExitWithError() is given an exit code and error string; and b) Exit(Nonzero) is changed to ExitWithError(Nonzero, "string") (when possible) Signed-off-by: Ed Santiago <[email protected]>

mheon · 2024-04-10T14:35:34Z

LGTM

Luap99 · 2024-04-10T14:55:42Z

/lgtm

Followup to containers#22270: wherever possible/practical, extend command error checks to include explicit exit status codes and error strings. This commit handles all remaining test/e2e/p*_test.go Signed-off-by: Ed Santiago <[email protected]>

Followup to containers#22270: wherever possible/practical, extend command error checks to include explicit exit status codes and error strings. This commit handles a subset of test/e2e/run_xxx_test.go Signed-off-by: Ed Santiago <[email protected]>

Followup to containers#22270: wherever possible/practical, extend command error checks to include explicit exit status codes and error strings. This commit handles test/e2e/s*_test.go Signed-off-by: Ed Santiago <[email protected]>

Followup to containers#22270: wherever possible/practical, extend command error checks to include explicit exit status codes and error strings. This commit handles all remaining test/e2e/r*_test.go Signed-off-by: Ed Santiago <[email protected]>

Followup to containers#22270: wherever possible/practical, extend command error checks to include explicit exit status codes and error strings. This commit handles only one file, test/e2e/rmi_test.go , because my changes are significant enough to merit individual review. Signed-off-by: Ed Santiago <[email protected]>

Final followup to containers#22270. That PR added a temporary convention allowing a new form of ExitWithError(), one with an exit code and stderr substring. In order to allow bite-size progress, the old no-args form was still allowed. This PR removes support for no-args ExitWithError(). This PR also adds one piece of new functionality: passing "" (empty string) as the stderr arg means "expect exit code but fail if there's anything at all in stderr". Signed-off-by: Ed Santiago <[email protected]>

openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 5, 2024

edsantiago marked this pull request as draft April 5, 2024 01:14

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2024

Luap99 reviewed Apr 5, 2024

View reviewed changes

edsantiago force-pushed the redefine-ExitWithError branch 5 times, most recently from 46e3908 to a43c29a Compare April 9, 2024 15:07

edsantiago changed the title ~~WIP: e2e: redefine ExitWithError() to require exit code~~ e2e: redefine ExitWithError() to require exit code Apr 9, 2024

edsantiago force-pushed the redefine-ExitWithError branch from a43c29a to 85f145f Compare April 9, 2024 16:17

edsantiago commented Apr 9, 2024

View reviewed changes

edsantiago removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 9, 2024

edsantiago marked this pull request as ready for review April 9, 2024 17:35

Luap99 reviewed Apr 10, 2024

View reviewed changes

edsantiago force-pushed the redefine-ExitWithError branch from 85f145f to 1b933a1 Compare April 10, 2024 12:08

edsantiago commented Apr 10, 2024

View reviewed changes

edsantiago force-pushed the redefine-ExitWithError branch from 1b933a1 to 2d91598 Compare April 10, 2024 12:36

openshift-ci bot assigned Luap99 Apr 10, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2024

openshift-merge-bot bot merged commit 17f36a3 into containers:main Apr 10, 2024
94 checks passed

edsantiago deleted the redefine-ExitWithError branch April 10, 2024 14:59

edsantiago mentioned this pull request May 8, 2024

ExitWithError() - rest of the p files #22648

Merged

edsantiago mentioned this pull request May 9, 2024

ExitWithError() - rmi_test #22660

Merged

edsantiago mentioned this pull request May 9, 2024

ExitWithError() - more r files #22661

Merged

edsantiago mentioned this pull request May 13, 2024

ExitWithError() - final push to strict mode #22690

Merged

stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Jul 10, 2024

stale-locking-app bot locked as resolved and limited conversation to collaborators Jul 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e: redefine ExitWithError() to require exit code #22270

e2e: redefine ExitWithError() to require exit code #22270

edsantiago commented Apr 5, 2024 •

edited

Loading

openshift-ci bot commented Apr 5, 2024

Luap99 left a comment

Luap99 Apr 5, 2024

Luap99 Apr 5, 2024 •

edited

Loading

mheon commented Apr 8, 2024

edsantiago commented Apr 8, 2024

edsantiago left a comment

edsantiago Apr 9, 2024

edsantiago Apr 9, 2024

Luap99 Apr 10, 2024

edsantiago Apr 10, 2024

Luap99 Apr 10, 2024

Luap99 commented Apr 10, 2024

edsantiago Apr 10, 2024

Luap99 Apr 10, 2024

edsantiago Apr 10, 2024

Luap99 Apr 10, 2024

mheon commented Apr 10, 2024

Luap99 commented Apr 10, 2024

		// FIXME: once all ExitWithError()s have been migrated to new form,
		// change interface to (int, ...string)

e2e: redefine ExitWithError() to require exit code #22270

e2e: redefine ExitWithError() to require exit code #22270

Conversation

edsantiago commented Apr 5, 2024 • edited Loading

openshift-ci bot commented Apr 5, 2024

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

mheon commented Apr 8, 2024

edsantiago commented Apr 8, 2024

edsantiago left a 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

Luap99 commented Apr 10, 2024

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

mheon commented Apr 10, 2024

Luap99 commented Apr 10, 2024

edsantiago commented Apr 5, 2024 •

edited

Loading

Luap99 Apr 5, 2024 •

edited

Loading