System tests: tighten 'is' operator #11776

edsantiago · 2021-09-28T21:00:49Z

Fix day-one sloppiness: when I first wrote this framework
it compared strings using 'expr', not '=', to be more
forgiving of extra cruft in output. This was a bad decision.
It means that warnings or additional text are ignored:

is "all is ok, NOT!"  "all is ok"  <-- this would pass

Solution: tighten up the 'is' check. Use '=' (direct
compare) first. If it fails, look for wild cards ('*')
or character classes ('[') in the expect string. If
so, and only then, use 'expr'. And, thanks to a clever
suggestion from Luap99, include '(using expr)' in the
error message when we do so; this could make it easier
for a developer to understand a string mismatch.

This change exposes a lot of instances in which we weren't
doing proper comparisons. Fix those. Thankfully, there
weren't as many as I'd feared.

Also, and completely unrelated, add '-T' flag to bats
helper, for showing timing results. (I will open this
as a separate PR if requested. I too find it offensive
to jumble together unrelated commits.)

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

openshift-ci · 2021-09-28T21:01:05Z

[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 · 2021-09-28T21:02:28Z

/hold

DO NOT MERGE. This cannot merge until #11775 is fixed! That's a serious nasty flake, and I bet this PR will hit it on most if not all of the podman-remote system test runs. I am submitting because this is (IMHO) code complete and ready for review.

edsantiago · 2021-09-28T22:27:50Z

Well, phooey! There's a new flake:

lstat /sys/fs/cgroup/devices/machine.slice/libpod-sha.scope: no such file or directory"'

I'll worry about it tomorrow.

edsantiago · 2021-09-28T22:30:32Z

And yet another different one:

level=warning msg="cannot toggle freezer: cgroups not configured for container"'
level=warning msg="lstat : no such file or directory"'

Luap99 · 2021-09-29T11:22:15Z

Nice work, that should help my brain to parse the errors.
Also the fact that you found new flakes proves the necessity of this.

edsantiago · 2021-09-29T13:37:59Z

This is blocked on ~~#11775~~, #11784, and ~~#11785~~.

giuseppe · 2021-09-30T14:18:39Z

#11784 is an issue in runc. I suggest we just skip the test with runc and move on

giuseppe · 2021-09-30T14:19:07Z

and probably also #10442

edsantiago

@containers/podman-maintainers I believe this is ready to merge. Thank you @rhatdan and @giuseppe for your work on the blockers.

edsantiago · 2021-09-30T17:00:46Z

test/system/005-info.bats

@@ -37,7 +37,7 @@ cgroupVersion: v[12]
    # FIXME: if we're ever able to get package versions on Debian,
    #        add '-[0-9]' to all '*.package' queries below.
    tests="
-host.buildahVersion       | [0-9.]
+host.buildahVersion       | [0-9.]\\\+


This will now fail if we ever vendor a -dev buildah, or something with letters.

We regularly do this during development, will this block a merge to upstream?

It probably would. Let me think of how to handle that; I will pore through git logs, find patterns, and resubmit. Thanks for letting me know.

edsantiago · 2021-09-30T17:02:55Z

test/system/125-import.bats

@@ -20,26 +20,26 @@ load helpers
    # Simple import
    run_podman import -q $archive
    iid="$output"
-    run_podman run -t --rm $iid cat /random.txt
+    run_podman run --rm $iid cat /random.txt


-t is tested in 450-interactive.bats (below), so I see little need to jump through the carriage-return hoops here. Please advise if there's something I've missed.

edsantiago · 2021-09-30T17:04:04Z

test/system/150-login.bats

@@ -245,7 +245,7 @@ function _test_skopeo_credential_sharing() {
    is "$status" "0" "skopeo inspect - exit status"

    got_name=$(jq -r .Name <<<"$output")
-    is "$got_name" "$registry/$dest_name" "skopeo inspect -> Name"
+    is "$got_name" "$registry/$destname" "skopeo inspect -> Name"


Oops. My bad. This has been broken from the beginning. This is a great example of why we need this PR.

edsantiago · 2021-09-30T17:06:08Z

test/system/160-volumes.bats

+    is "$(echo $(sort <<<$output))" "${v[4]} ${v[5]} ${v[6]}" \
+       "volume prune, with 1, 2, 3 in use, deletes only 4, 5, 6"


This is another great example of why we need this PR. Between the time I wrote this test and today, someone added some new volumes, and this check was not catching the addition.

edsantiago · 2021-09-30T17:07:29Z

test/system/700-play.bats

@@ -30,7 +30,7 @@ spec:
  containers:
  - command:
    - sleep
-    - "100"
+    - \"100\"


This and line 52 are completely unrelated; it's just something that makes color highlighting work better in my editors.

TomSweeneyRedHat · 2021-09-30T18:01:38Z

LGTM
and happy green test buttons
thx again for the gr8 work @edsantiago !

edsantiago · 2021-09-30T19:12:13Z

/hold
the -dev thing is much more tedious than I had expected

Fix day-one sloppiness: when I first wrote this framework it compared strings using 'expr', not '=', to be more forgiving of extra cruft in output. This was a bad decision. It means that warnings or additional text are ignored: is "all is ok, NOT!" "all is ok" <-- this would pass Solution: tighten up the 'is' check. Use '=' (direct compare) first. If it fails, look for wild cards ('*') or character classes ('[') in the expect string. If so, and only then, use 'expr'. And, thanks to a clever suggestion from Luap99, include '(using expr)' in the error message when we do so; this could make it easier for a developer to understand a string mismatch. This change exposes a lot of instances in which we weren't doing proper comparisons. Fix those. Thankfully, there weren't as many as I'd feared. Also, and completely unrelated, add '-T' flag to bats helper, for showing timing results. (I will open this as a separate PR if requested. I too find it offensive to jumble together unrelated commits.) Signed-off-by: Ed Santiago <[email protected]>

edsantiago · 2021-09-30T19:35:54Z

test/system/005-info.bats

@@ -37,7 +37,7 @@ cgroupVersion: v[12]
    # FIXME: if we're ever able to get package versions on Debian,
    #        add '-[0-9]' to all '*.package' queries below.
    tests="
-host.buildahVersion       | [0-9.]
+host.buildahVersion       | [1-9][0-9]*\.[0-9.]\\\+.*


Update on the -dev issue: I confirmed (back to 1.4) that the only values are N.M and N.M-dev, but then ended up unable to use expr to handle -dev. So the above expression now handles N.M with any suffix, such as 1.23.0-dev but also 1.23abcfoothisisnonsensehellogoodbye. I think we'll need to call that Good Enough.

edsantiago · 2021-09-30T20:52:09Z

Ready again.

If there are in-flight PRs which add BATS tests that do lax checking, and they merge without rebasing, we may end up with a broken main. I've looked through the top half of open PRs and see none, but I might have missed one.

vrothberg

/lgtm

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

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2021

edsantiago added do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 28, 2021

edsantiago mentioned this pull request Sep 29, 2021

cgroupsv1(?): cannot toggle freezer: cgroups not configured for container #11785

Closed

edsantiago force-pushed the bats_is_cleanup branch from ea3379f to f0db4ea Compare September 30, 2021 14:12

edsantiago force-pushed the bats_is_cleanup branch 2 times, most recently from 2ab71bf to 7e77927 Compare September 30, 2021 15:39

edsantiago commented Sep 30, 2021

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2021

edsantiago force-pushed the bats_is_cleanup branch from 7e77927 to bf94ebf Compare September 30, 2021 19:32

edsantiago commented Sep 30, 2021

View reviewed changes

edsantiago removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2021

vrothberg reviewed Oct 1, 2021

View reviewed changes

openshift-ci bot assigned vrothberg Oct 1, 2021

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

openshift-merge-robot merged commit 317e20a into containers:main Oct 1, 2021

edsantiago deleted the bats_is_cleanup branch October 1, 2021 11:27

edsantiago mentioned this pull request Oct 4, 2021

podman create: Unable to close conn: \"close unixpacket @->/proc/self/fd/11/attach: use of closed network connection #11856

Closed

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

System tests: tighten 'is' operator #11776

System tests: tighten 'is' operator #11776

edsantiago commented Sep 28, 2021

openshift-ci bot commented Sep 28, 2021

edsantiago commented Sep 28, 2021

edsantiago commented Sep 28, 2021

edsantiago commented Sep 28, 2021

Luap99 commented Sep 29, 2021

edsantiago commented Sep 29, 2021 •

edited

Loading

giuseppe commented Sep 30, 2021

giuseppe commented Sep 30, 2021

edsantiago left a comment

edsantiago Sep 30, 2021

rhatdan Sep 30, 2021

edsantiago Sep 30, 2021

edsantiago Sep 30, 2021

edsantiago Sep 30, 2021

edsantiago Sep 30, 2021

edsantiago Sep 30, 2021

TomSweeneyRedHat commented Sep 30, 2021

edsantiago commented Sep 30, 2021

edsantiago Sep 30, 2021

edsantiago commented Sep 30, 2021

vrothberg left a comment

		is "$(echo $(sort <<<$output))" "${v[4]} ${v[5]} ${v[6]}" \
		"volume prune, with 1, 2, 3 in use, deletes only 4, 5, 6"

System tests: tighten 'is' operator #11776

System tests: tighten 'is' operator #11776

Conversation

edsantiago commented Sep 28, 2021

openshift-ci bot commented Sep 28, 2021

edsantiago commented Sep 28, 2021

edsantiago commented Sep 28, 2021

edsantiago commented Sep 28, 2021

Luap99 commented Sep 29, 2021

edsantiago commented Sep 29, 2021 • edited Loading

giuseppe commented Sep 30, 2021

giuseppe commented Sep 30, 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

TomSweeneyRedHat commented Sep 30, 2021

edsantiago commented Sep 30, 2021

Choose a reason for hiding this comment

edsantiago commented Sep 30, 2021

vrothberg left a comment

Choose a reason for hiding this comment

edsantiago commented Sep 29, 2021 •

edited

Loading