Semiperiodic cleanup of obsolete Skip()s #12349

edsantiago · 2021-11-18T16:33:17Z

Found by my find-obsolete-skips script. Let's see which, if any,
of these skipped tests can be reenabled.

Some Skips are "this will never work", not "this is expected to
work one day". Update the message on those to reflect that.

Signed-off-by: Ed Santiago [email protected]

openshift-ci · 2021-11-18T16:33:20Z

[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

edsantiago

Added annotations for my changes. (As help for reviewers. None of these comments are appropriate for the code itself).

Still a WIP because the system test hasn't run.

edsantiago · 2021-11-18T22:23:51Z

test/e2e/common_test.go

@@ -320,7 +320,7 @@ func (p *PodmanTestIntegration) createArtifact(image string) {
 	}
 	dest := strings.Split(image, "/")
 	destName := fmt.Sprintf("/tmp/%s.tar", strings.Replace(strings.Join(strings.Split(dest[len(dest)-1], "/"), ""), ":", "-", -1))
-	fmt.Printf("Caching %s at %s...", image, destName)
+	fmt.Printf("Caching %s at %s...\n", image, destName)


This is immediately followed by a verbose podman pull, which spits out Running: etc on the same line. My logformatter does not see this command and does not perform its helpful highlights.

edsantiago · 2021-11-18T22:24:29Z

test/e2e/logs_test.go

@@ -214,7 +214,7 @@ var _ = Describe("Podman logs", func() {

 		It("two containers showing short container IDs: "+log, func() {
 			skipIfJournaldInContainer()
-			SkipIfRemote("FIXME: podman-remote logs does not support showing two containers at the same time")
+			SkipIfRemote("podman-remote logs does not support showing two containers at the same time")


Removed the FIXME, because this does not look like it's going to be fixed

edsantiago · 2021-11-18T22:24:47Z

test/e2e/push_test.go

@@ -95,7 +95,7 @@ var _ = Describe("Podman push", func() {
 	})

 	It("podman push to local registry with authorization", func() {
-		SkipIfRootless("FIXME: Creating content in certs.d we use directories in homedir")
+		SkipIfRootless("volume-mounting a certs.d file N/A over remote")


Removed the FIXME, because this does not look like it's going to be fixed

edsantiago · 2021-11-18T22:26:03Z

test/e2e/run_cgroup_parent_test.go

@@ -48,21 +48,22 @@ var _ = Describe("Podman run with --cgroup-parent", func() {
 		run := podmanTest.Podman([]string{"run", "--cgroupns=host", "--cgroup-parent", cgroup, fedoraMinimal, "cat", "/proc/self/cgroup"})
 		run.WaitWithDefaultTimeout()
 		Expect(run).Should(Exit(0))
-		ok, _ := run.GrepString(cgroup)
-		Expect(ok).To(BeTrue())
+		Expect(run.OutputToString()).To(ContainSubstring(cgroup))


I really can't begin to express how anti-helpful it is to see "zero is not one" in ginkgo error logs.

edsantiago · 2021-11-18T22:27:29Z

test/e2e/run_cleanup_test.go

@@ -35,7 +35,7 @@ var _ = Describe("Podman run exit", func() {

 	It("podman run -d mount cleanup test", func() {
 		SkipIfRemote("podman-remote does not support mount")
-		SkipIfRootless("FIXME podman mount requires podman unshare first")
+		SkipIfRootless("TODO rootless podman mount requires podman unshare first")


Changed FIXME to TODO. The difference, as I see it, is that TODO is "that would take effort. Maybe some day we'll enhance the test". Whereas FIXME is "this is broken". (And my find-obsolete-skips script flags FIXME but not TODO)

edsantiago · 2021-11-18T22:29:10Z

test/e2e/run_signal_test.go

@@ -45,7 +45,6 @@ var _ = Describe("Podman run with --sig-proxy", func() {
 	})

 	Specify("signals are forwarded to container using sig-proxy", func() {
-		SkipIfRemote("FIXME: This looks like it is supposed to work in remote")


Fixed by the PodmanPID change at top

edsantiago · 2021-11-18T22:29:30Z

test/e2e/run_test.go

@@ -681,7 +681,7 @@ USER bin`, BB)
 	})

 	It("podman run device-read-bps test", func() {
-		SkipIfRootless("FIXME: Missing /sys/fs/cgroup/user.slice/user-14467.slice/[email protected]/cgroup.subtree_control")
+		SkipIfRootless("FIXME: requested cgroup controller `io` is not available")


This is the new error message as of this writing.

edsantiago · 2021-11-18T22:29:53Z

test/e2e/run_volume_test.go

@@ -282,8 +282,8 @@ var _ = Describe("Podman run with volumes", func() {
 	})

 	It("podman run with tmpfs named volume mounts and unmounts", func() {
-		SkipIfRootless("FIXME:  rootless podman mount requires you to be in a user namespace")
-		SkipIfRemote("podman-remote does not support --volumes this test could be simplified to be tested on Remote.")
+		SkipIfRootless("rootless podman mount requires you to be in a user namespace")


Removed the FIXME. Maybe this should be a TODO instead?

edsantiago · 2021-11-18T22:30:11Z

test/system/030-run.bats

@@ -236,7 +236,7 @@ echo $rand        |   0 | $rand
 }

 @test "podman run docker-archive" {
-    skip_if_remote "podman-remote does not support docker-archive (#7116)"
+    skip_if_remote "podman-remote does not support docker-archive"


This will never be fixed. Remove the reference.

edsantiago · 2021-11-18T22:30:35Z

test/utils/utils.go

@@ -121,6 +121,7 @@ func (p *PodmanTest) WaitForContainer() bool {
 		}
 		time.Sleep(1 * time.Second)
 	}
+	fmt.Printf("WaitForContainer(): timed out\n")


This would've been really helpful when I was debugging the PodmanPID bug above.

mheon · 2021-11-19T14:35:20Z

LGTM

Found by my find-obsolete-skips script. Let's see which, if any, of these skipped tests can be reenabled. Some Skips are "this will never work", not "this is expected to work one day". Update the message on those to reflect that. Some were real bugs in the test framework. Fix those. And, joy of joys, some work today. Remove those skips. Signed-off-by: Ed Santiago <[email protected]>

umohnani8 · 2021-11-22T15:58:07Z

LGTM

rhatdan · 2021-11-22T16:50:01Z

/lgtm

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 18, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2021

edsantiago force-pushed the clean_fixmes branch 4 times, most recently from 507bfc7 to f1e7852 Compare November 18, 2021 22:17

edsantiago commented Nov 18, 2021

View reviewed changes

edsantiago force-pushed the clean_fixmes branch 2 times, most recently from aebda50 to b72de12 Compare November 19, 2021 03:12

edsantiago changed the title ~~WIP: Semiperiodic cleanup of obsolete Skip()s~~ Semiperiodic cleanup of obsolete Skip()s Nov 19, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 19, 2021

edsantiago force-pushed the clean_fixmes branch from b72de12 to c03b6b5 Compare November 19, 2021 16:04

edsantiago mentioned this pull request Nov 20, 2021

[remote] error creating cgroup [...]: dial unix /run/systemd/private: connect: EPERM #12362

Closed

openshift-ci bot assigned rhatdan Nov 22, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2021

openshift-merge-robot merged commit 40ae7e7 into containers:main Nov 22, 2021

edsantiago deleted the clean_fixmes branch November 22, 2021 16:53

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Semiperiodic cleanup of obsolete Skip()s #12349

Semiperiodic cleanup of obsolete Skip()s #12349

edsantiago commented Nov 18, 2021

openshift-ci bot commented Nov 18, 2021

edsantiago left a comment

edsantiago Nov 18, 2021

edsantiago Nov 18, 2021

edsantiago Nov 18, 2021

edsantiago Nov 18, 2021

edsantiago Nov 18, 2021

edsantiago Nov 18, 2021

edsantiago Nov 18, 2021

edsantiago Nov 18, 2021

edsantiago Nov 18, 2021

edsantiago Nov 18, 2021

mheon commented Nov 19, 2021

umohnani8 commented Nov 22, 2021

rhatdan commented Nov 22, 2021

Semiperiodic cleanup of obsolete Skip()s #12349

Semiperiodic cleanup of obsolete Skip()s #12349

Conversation

edsantiago commented Nov 18, 2021

openshift-ci bot commented Nov 18, 2021

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

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

mheon commented Nov 19, 2021

umohnani8 commented Nov 22, 2021

rhatdan commented Nov 22, 2021