Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

e2e tests: use HaveKey() and HaveLen() when possible #12482

Merged
merged 7 commits into from
Dec 2, 2021

Conversation

edsantiago
Copy link
Member

@edsantiago edsantiago commented Dec 2, 2021

I learned about HaveKey() and HaveLen(). We should use them wherever possible.

This is another multi-part PR, in small reviewable chunks.

  • e2e tests: a little more minor cleanup
  • Same as previous, for assertions other than Equal()
  • Use BeEmpty() instead of len(x).To(Equal(0))
  • Same thing, for BeNumerically("==", 0)
  • Use HaveLen(x) instead of Expect(len(y)).To(Equal(x))
  • Same thing, with BeNumerically("==", x)
  • Manual fixes

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 2, 2021

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2021
@edsantiago
Copy link
Member Author

Sample output using the new HaveLen():

  Expected
      <[]string | len:6, cap:8>: [
          "REPOSITORY                         TAG         IMAGE ID      CREATED        SIZE        R/O",
          "quay.io/libpod/alpine_nginx        latest      7397e078c6d9  9 months ago   14.2 MB     true",
          "quay.io/libpod/redis               alpine      8835e6aeca99  13 months ago  32.3 MB     true",
          "quay.io/libpod/alpine              latest      961769676411  2 years ago    5.85 MB     true",
          "quay.io/libpod/alpine_healthcheck  latest      c1103abb47d6  2 years ago    10.4 MB     true",
          "quay.io/libpod/alpine_labels       latest      4fab981df737  3 years ago    4.68 MB     true",
      ]
  to have length 8

Compare to:

  Expected
      <int>: 6
  to equal
      <int>: 8

@mheon
Copy link
Member

mheon commented Dec 2, 2021

LGTM once things go green.

@edsantiago
Copy link
Member Author

I need help here. (I could figure it out myself, but it'll be faster and more reliable for me to ask). The error is:

         Expected
               <map[string]string | len:2>: {
                   "io.buildah.version": "1.23.1",
                   "marge": "mom",
               }
           to have {key: value}
               <map[interface {}]interface {} | len:1>: {
                   <string>"homer": <string>"",
               }

The cause is this diff:

-               Expect(inspectData[0].Config.Labels["homer"]).To(Equal(""))
+               Expect(inspectData[0].Config.Labels).To(HaveKeyWithValue("homer", ""))

My initial interpretation is: Go returns "" (empty string) when looking up a nonexistent key in a map. Is that correct? If so, should the replacement assertion be ...To(Not(HaveKey("homer")))?

  sed -i -e 's/Expect(\(.*\)\[\(\".*\"\)\])\.To(Equal(/Expect(\1).To(HaveKeyWithValue(\2, /' test/e2e/*_test.go

...with two manual tweaks, because this converted:

    Expect(foo["bar"]).To(Equal(""))
 -> Expect(foo).To(HaveKeyWithValue("bar",""))

It looks like the intention of the test was, instead:

    ...To(Not(HaveKey("bar")))

Signed-off-by: Ed Santiago <[email protected]>
  sed -i -e 's/Expect(\(.*\)\[\(\".*\"\)\])\.To(\(.*\)/Expect(\1).To(HaveKeyWithValue(\2, \3)/' test/e2e/*_test.go

Signed-off-by: Ed Santiago <[email protected]>
  sed -i -e 's/Expect(len(\(.*\)))\.To(Equal(0))/Expect(\1).To(BeEmpty())/' test/e2e/*.go

Signed-off-by: Ed Santiago <[email protected]>
  sed -i -e 's/Expect(len(\(.*\)))\.To(BeNumerically("==", 0))/Expect(\1).To(BeEmpty())/' test/e2e/*.go

Signed-off-by: Ed Santiago <[email protected]>
  sed -i -e 's/Expect(len(\(.*\)))\.To(Equal(\(.*\)))/Expect(\1).To(HaveLen(\2))/' test/e2e/*.go

Signed-off-by: Ed Santiago <[email protected]>
  sed -i -e 's/Expect(len(\(.*\)))\.To(BeNumerically("==", \(.*\)))/Expect(\1).To(HaveLen(\2))/' test/e2e/*.go

Signed-off-by: Ed Santiago <[email protected]>
Fix a handful of instances not covered by earlier automated
replacements. Found via:

   ack 'Expect\(len' test/e2e

There are still a bunch of BeNumerically(">", ...) that cannot (yet)
be handled by HaveLen(). Leave those as they are.

Signed-off-by: Ed Santiago <[email protected]>
@edsantiago
Copy link
Member Author

Re-pushed on the assumption that my above comment is correct. Edit was to the first commit. I amended the commit message appropriately, to indicate manual muckery.

@edsantiago edsantiago changed the title e2e tests: use HaveLen() when possible e2e tests: use HaveKey() and HaveLen() when possible Dec 2, 2021
@rhatdan
Copy link
Member

rhatdan commented Dec 2, 2021

LGTM
@containers/podman-maintainers PTAL

@mheon
Copy link
Member

mheon commented Dec 2, 2021

Your assumption is correct, Ed

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2021
@openshift-merge-robot openshift-merge-robot merged commit 87de344 into containers:main Dec 2, 2021
@edsantiago edsantiago deleted the e2e_have branch December 2, 2021 19:11
@github-actions 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 github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants