e2e tests: use HaveField() for better error checking

[edsantiago]

This is a very late followup to my ginkgo-improving work of 2021.
It has been stuck since December because it requires gomega 1.17,
which we've just enabled.

This commit is simply a copy-paste of a command I saved in
my TODO list many months ago:

 sed -i -e 's/Expect(\([^ ]\+\)\.\([a-zA-Z0-9]\+\))\.To(Equal(/Expect(\1).To(HaveField(\"\2\", /' test/e2e/*_test.go

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

[openshift-ci]

[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]

Two commits, for ease of review (see commit message of 2nd commit for reason).

Tested by hand-mucking with init_test.go. Old results:

Expected
      <string>: initialized
  to equal
      <string>: initializing

New:

 Value for field 'Status' failed to satisfy matcher.      <<<<---- this is the diff
  Expected
      <string>: initialized
  to equal
      <string>: initializing

And, should it ever happen that the struct being tested does not even have an element with that name:

HaveField could not find field named 'Xtatus' in struct:
      <*define.InspectContainerState | 0xc0003d2000>: {
          OciVersion: "1.0.2-dev",
          Status: "initialized",
          Running: false,
          Paused: false,
          Restarting: false,
          OOMKilled: false,
          Dead: false,
          Pid: 1613918,
          ConmonPid: 1613915,
          ExitCode: 0,
          Error: "",
          StartedAt: 0001-01-01T00:00:00Z,
          FinishedAt: 0001-01-01T00:00:00Z,
          Health: {Status: "", FailingStreak: 0, Log: nil},
          Checkpointed: false,
          CgroupPath: "",
          CheckpointedAt: 0001-01-01T00:00:00Z,
          RestoredAt: 0001-01-01T00:00:00Z,
          CheckpointLog: "",
          CheckpointPath: "",
          RestoreLog: "",
          Restored: false,
      }

[edsantiago]

This is really annoying:

  Value for field 'Value' failed to satisfy matcher.
  Expected
      <*string | 0xc000504380>: blue
  to equal
      <string>: blue

I've tried BeEquivalent() and BeEqual() and a variety of other dead ends. Does anyone know if there's an incantation for dereferencing Value, or for telling HaveField() to just shut up and Do What I Mean?

[vrothberg]

The below diff turned it green on my end:

diff --git a/test/e2e/generate_kube_test.go b/test/e2e/generate_kube_test.go                         
index f58b121f3070..31f2ea16e927 100644                                                              
--- a/test/e2e/generate_kube_test.go                                                                 
+++ b/test/e2e/generate_kube_test.go                                                                 
@@ -777,7 +777,8 @@ var _ = Describe("Podman generate kube", func() {                                
                Expect(pod.Spec.DNSConfig.Searches).To(ContainElement("foobar.com"))                 
                Expect(len(pod.Spec.DNSConfig.Options)).To(BeNumerically(">", 0))                    
                Expect(pod.Spec.DNSConfig.Options[0]).To(HaveField("Name", "color"))                 
-               Expect(pod.Spec.DNSConfig.Options[0]).To(HaveField("Value", "blue"))                 
+               s := "blue"                                                                          
+               Expect(pod.Spec.DNSConfig.Options[0]).To(HaveField("Value", &s))                     
        })                                                                                           
                                                                                                     
        It("podman generate kube multiple container dns servers and options are cumulative", func() {

Probably worth adding a helper function somewhere (utils package?) for such cases:

func StringToPointer(s string) *string {
     return &s
}

[vrothberg]

Changes LGTM, really nice!

[Luap99]

LGTM but there is one downside with this function.

When I change the struct field the compiler/IDE would complain that the test is using a field which no longer exists. Now I will run the tests to get a failure much later.

[edsantiago]

The Bad: new registry flake.

The Good: the only files touched in this PR are in test/e2e, so the following tests were all skipped: Compose, Compose v2, Unit, System, bud, upgrade, all with:

SKIPPING: This is a doc- and/or test-only PR with no changes under [whatever]

The Neutral: I have no opinion on the struct-change issue brought up by @Luap99, and will leave it for the team to weigh the tradeoffs. If struct renames are that common, please just close this without merging.

Thanks @vrothberg for the pointer fix!

[Luap99]

@edsantiago This not a problem for me I just wanted to point that out.
We should not rename the fields often. The error message is much more helpful this way so I like this.

[rhatdan]

/lgtm