e2e tests: clean up antihelpful BeTrue()s #12387
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
[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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
edsantiago
commented
Nov 22, 2021
| @@ -186,23 +180,22 @@ var _ = Describe("Podman UserNS support", func() { | |||
| session := podmanTest.Podman([]string{"run", "--userns=auto:size=500", "alpine", "cat", "/proc/self/uid_map"}) | |||
| session.WaitWithDefaultTimeout() | |||
| Expect(session).Should(Exit(0)) | |||
| ok, _ := session.GrepString("500") | |||
There was a problem hiding this comment.
This, and (old) lines 194, 199 required manual fixing (my script could not identify them). Unless I'm missing something major, I believe these three lines were complete NOPs.
|
Aside from the above comment, all changes (at this point) were generated by a script. Once (if?) CI passes I will post the content of said script, for future posterity. |
|
I like this but tests are failing. |
|
Yeah; And, Static Build seems completely broken, something to do with nix. I've restarted it four times, no luck. |
Luap99
reviewed
Nov 22, 2021
test/e2e/containers_conf_test.go
Outdated
| @@ -252,7 +252,7 @@ var _ = Describe("Podman run", func() { | |||
| session := podmanTest.Podman([]string{"run", ALPINE, "cat", "/etc/resolv.conf"}) | |||
| session.WaitWithDefaultTimeout() | |||
| Expect(session).Should(Exit(0)) | |||
| Expect(session.LineInOutputStartsWith("search")).To(BeTrue()) | |||
| Expect(session.OutputToString()).To(HavePrefix("search")) | |||
There was a problem hiding this comment.
This is not correct, LineInOutputStartsWith checks each line individually for this prefix while your change only works for the first line.
There was a problem hiding this comment.
yes, exactly. I'm looking for a Ginkgo replacement.
There was a problem hiding this comment.
I think you need to write your own custom matcher
edsantiago
force-pushed
the
remove_betrue
branch
2 times, most recently
from
031a263 to
34791da
Compare
Many ginkgo tests have been written to use this evil form:
GrepString("foo")
Expect(that to BeTrue())
...which yields horrible useless messages on failure:
false is not true
Identify those (automatically, via script) and convert to:
Expect(output to ContainSubstring("foo"))
...which yields:
"this output" does not contain substring "foo"
There are still many BeTrue()s left. This is just a start.
This is commit 1 of 2. It includes the script I used, and
all changes to *.go are those computed by the script.
Commit 2 will apply some manual fixes.
Signed-off-by: Ed Santiago <[email protected]>
edsantiago
force-pushed
the
remove_betrue
branch
from
34791da to
260c8e3
Compare
|
/lgtm |
openshift-ci
bot
added
the
do-not-merge/hold
Commit 2 of 2: there were (still are?) a bunch of string
checks that didn't have a corresponding Expect(). IIUC
that means they were NOPs. Try to identify and fix those.
The first few were caught by Go linting, "ok is defined
but not used". When I realized the problem, I looked for
more using:
$ ack -A2 LineInOutputStartsWith
...and tediously eyeballing the results, looking for
matches in which the next line was not Expect(). If
test was wrong (e.g. "server" should've been "nameserver"),
fix that.
Also: remove the remove-betrue script. We don't need it
in the repo, I just wanted to preserve it for posterity.
Signed-off-by: Ed Santiago <[email protected]>
edsantiago
force-pushed
the
remove_betrue
branch
from
260c8e3 to
49d63ad
Compare
edsantiago
commented
Nov 22, 2021
| }) | ||
|
|
||
| It("podman run add dns server", func() { | ||
| session := podmanTest.Podman([]string{"run", ALPINE, "cat", "/etc/resolv.conf"}) | ||
| session.WaitWithDefaultTimeout() | ||
| Expect(session).Should(Exit(0)) | ||
| session.LineInOutputStartsWith("server 1.2.3.4") | ||
| Expect(session.OutputToStringArray()).To(ContainElement(HavePrefix("nameserver 1.2.3.4"))) |
There was a problem hiding this comment.
Another manual (not automated) change: this (and the other copy/paste instance in run_dns_test.go below) was broken from before, but the test was a NOP so it was never caught.
|
OK, I'm done for today but I think this final iteration should pass CI. This is nearly impossible to review, but I would appreciate eyeballs on the second commit (the Oops one) because you should be really scared by what you find there: a small number of tests that were doing nothing whatsoever. And I suspect there may be more, but they're super hard to find and I can't think of any automated way to detect them. |
|
I want another pair of eyes to parse the new changes before merging. |
Luap99
reviewed
Nov 23, 2021
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
Thanks everyone!
That was my initial goal. It's not quite that simple: Lines 280 to 283 in 90c635f I will be following up on it, though. I will be doing so in medium-size reviewable chunks. |
|
I am wondering if the linter is running on the test/e2e directory. Normally the linter will complain if the return value is unused. |
|
It is (running): that's how I found the initial bugs that led me to the second commit. My script removed an |
|
@edsantiago I think this is just the go compiler complaining about an unused variable not the actual linter. |
|
@vrothberg FYI, for baseline purposes: here are
|
|
@edsantiago The linter is not running: The interesting failures are the |
github-actions
bot
added
the
locked - please file new issue/PR
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Many ginkgo tests have been written to use this evil form:
...which yields horrible useless messages on failure:
Identify those (automatically, via script) and convert to:
...which yields:
There are still many BeTrue()s left. This is just a start.
Signed-off-by: Ed Santiago [email protected]