continue e2e test cleanup #12398
continue e2e test cleanup #12398
Conversation
|
Four separate commits; I recommend reviewing each independently. And, there are some FIXMEs that I need help with. TIA. |
openshift-ci
bot
added
do-not-merge/work-in-progress
edsantiago
force-pushed
the
remove_betrue
branch
2 times, most recently
from
08b6b25 to
7c29d29
Compare
| @@ -554,7 +542,7 @@ VOLUME /test/`, ALPINE) | |||
| Skip("Overlay mounts not supported when running in a container") | |||
| } | |||
| if rootless.IsRootless() { | |||
| if _, err := exec.LookPath("fuse_overlay"); err != nil { | |||
| if _, err := exec.LookPath("fuse-overlayfs"); err != nil { | |||
There was a problem hiding this comment.
This was a typo. Tests were never running rootless, because there is no such binary as fuse_overlay.
FIXME: is fuse-overlayfs needed even in the new world where overlay is supported in the kernel? Does this conditional need to change?
There was a problem hiding this comment.
I agree, we should run the tests no matter what.
There was a problem hiding this comment.
Yes i remember changing it for some new tests https://github.com/containers/podman/pull/11650/files but it should be done across the code.
|
@edsantiago I know it is tedious to fix all the lint errors but can you add this diff and fix the errors: diff --git a/.golangci.yml b/.golangci.yml
index cf067a58c..f3338b9ae 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -6,7 +6,6 @@ run:
skip-dirs:
- contrib
- dependencies
- - test
skip-files:
- swagger.go
modules-download-mode: readonly |
|
@Luap99 not in this PR: this is already close to what I consider the limit of unreviewability. I will address it in a followup PR. |
|
@edsantiago sure, thanks |
|
@containers/podman-maintainers PTAL, especially at my three questions. (I'm not going to bother re-running the failed system test. It is a flake, and irrelevant for purposes of this PR) |
Continue eliminating GrepString() and BeTrue(), in tiny incremental steps. Here I take the liberty of refactoring some hard-to-read code by adding a helper. Signed-off-by: Ed Santiago <[email protected]>
via: sed -i -e 's/Expect(StringInSlice(\(.*\), \(.*\))).To(BeTrue())/Expect(\2)\.To(ContainElement(\1))/' test/e2e/*_test.go Signed-off-by: Ed Santiago <[email protected]>
These were NOPs, and were testing the wrong thing (pod ID, not container ID). Fixed manually. Signed-off-by: Ed Santiago <[email protected]>
edsantiago
force-pushed
the
remove_betrue
branch
from
7c29d29 to
eb3b779
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
That previous commit made me wonder if there are any other instances of Expect() with no assertions. grep Expect test/e2e/*_test.go |egrep -v '\.(To|NotTo|Should)' ...finds a couple of handfuls, most of which are OK (continued on the next line) but a few of which are bugs. Fix those. Signed-off-by: Ed Santiago <[email protected]>
edsantiago
force-pushed
the
remove_betrue
branch
from
eb3b779 to
eb3708a
Compare
|
LGTM |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, giuseppe 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 |
github-actions
bot
added
the
locked - please file new issue/PR
Some more cleanup of assertions in e2e tests.
The original intent was maintainability: make it so tests emit useful diagnostics. But in the process I discovered various tests that were NOPs. I've tried to fix those to the best of my ability, but some need further tweaking.