replace ExpectEqual(someBoolean, ...)

[pohly]

What would you like to be added?

The test/e2e tests are full of assertions which compare a boolean against true or false, often without any additional explanation. When those assertions fail, the error message is useless for understanding what went wrong, often just saying:

      Apr 22 09:03:41.130: Expected
          <bool>: false
      to equal
          <bool>: true

Including an explanation is better, but then we still have the distracting noise about the comparison:

      Apr 22 09:08:49.598: Failed to do XYZ
      Expected
          <bool>: false
      to equal
          <bool>: true

All of those assertions should be replace with if (<failure check>) framework.Failf(<informative message>).

Such locations can be found by grepping:

$ git grep 'Expect.*Equal([^,]*, *\(true\|false\)' | wc -l
272

Some progress has been made in the meantime:

• v1.23.0: 250
• v1.24.0-beta.0: 177

However, we were down to 175 at some point, so the number is going back up.

Example:

test/e2e/storage/vsphere/vsphere_volume_diskformat.go:  framework.ExpectEqual(isAttached, true)
=>
if (!isAttached) {
   framework.Failf("volume %s is not attached", ...)
}

Some other places compare values. Those should be replaced with a comparison assertion. Example:

test/e2e/upgrades/apps/etcd.go: framework.ExpectEqual(ratio > 0.75, true)
=>
gomega.Expect(ratio).To(gomega.BeNumerically(">", 0.75), "ratio too small")

It makes sense to split this work into individual PRs, one per OWNERS file under test/e2e. Here's a list of directories:

• test/e2e/apimachinery @hunknownz
• test/e2e/apps: Changed code to improve output messages on error for files under test/e2e/apps  #109944
• test/e2e/auth
• test/e2e/autoscaling: Changed code to improve output for files under test/e2e/autoscaling #106200
• test/e2e/cloud/gcp
• test/e2e/cloud
• test/e2e/common/network
• test/e2e/common/node: @jwtty
• test/e2e/common/storage: test/e2e/common/storage: enhance assertions #106624
• test/e2e/framework: Changed code to improve output for files under test/e2e/framework #105939
• test/e2e/framework/providers/vsphere
• test/e2e/framework/volume
• test/e2e/instrumentation/logging
• test/e2e/instrumentation: Changed code to improve output for files under test/e2e/instrumentation #106233
• test/e2e/kubectl
• test/e2e/lifecycle
• test/e2e/node: test/e2e/node + test/e2e/scheduling: improve checks #106486
• test/e2e/scheduling: test/e2e/node + test/e2e/scheduling: improve checks #106486
• test/e2e/storage: @SD-13 (prior PR: Improved the quality of output when the test fails #105860)
• test/e2e/testing-manifests/storage-csi
• test/e2e/upgrades/apps: Better output #105774
• test/e2e/windows: Changed code to improve output for test/e2e/windows #106220, enhance assertions in test/e2e/windows #110303

When creating a PR, use <directory>: enhance assertions as title. Use one commit per PR, squash when addressing feedback. Unless noted otherwise, @chetak123 will work on PRs. Help may be welcome.

/sig testing
/help

Why is this needed?

Better output for developers when tests fail.

[k8s-ci-robot]

@pohly:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

What would you like to be added?

The test/e2e tests are full of assertions which compare a boolean against true or false, often without any additional explanation. When those assertions fail, the Ginkgo error message is useless for understanding what went wrong, basically just saying "expected false to be true".

All of those assertions should be replace with if (<failure check>) framework.Failf(<informative message>).

Such locations can be found by grepping:

$ git grep 'Expect.*Equal([^,]*, *\(true\|false\)' | wc -l
272

Example:

test/e2e/storage/vsphere/vsphere_volume_diskformat.go:  framework.ExpectEqual(isAttached, true)
=>
if (!isAttached) {
  framework.Failf("volume %s is not attached", ...)
}

Some other places compare values. Those should be replaced with a comparison assertion. Example:

test/e2e/upgrades/apps/etcd.go: framework.ExpectEqual(ratio > 0.75, true)
=>
gomega.Expect(ratio).To(gomega.BeNumerically(">", 0.75), "ratio too small")

/sig testing
/help

Why is this needed?

Better output for developers when tests fail.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[aojea]

/good-first-issue

[k8s-ci-robot]

@aojea:
This request has been marked as suitable for new contributors.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[BenTheElder]

/triage accepted

[rishabhkumar296]

I would like to take this up as my first issue

[chetak123]

/assign @chetak123

[BurhanuddinMerchant]

I would like to take up this issue as my first issue

[chetak123]

Hello @BurhanuddinMerchant I am already working on this issue , can you please take some other issue ?

[BurhanuddinMerchant]

Hello @BurhanuddinMerchant I am already working on this issue , can you please take some other issue ?

Okay sure ,how do I un assign myself from this issue?

[chetak123]

I don't think you are assigned to this issue.

[BurhanuddinMerchant]

Oh okay

[chetak123]

Hey @NikhilSharmaWe as discussed in pr #105860 (comment) I am assuming that you are only working for tests under test/e2e/storage area.

[pohly]

@NikhilSharmaWe also finished the work on test/e2e/upgrades/apps, see #105774

[NikhilSharmaWe]

@pohly and @chetak123 only one PR is to be made for the correction under test/e2e/storage/, so how can we distribute the task and also make only single PR. Or can we make multiple PR's

[pohly]

I have added a TODO list to the issue description. I'll try to keep that up-to-date with what has been done and who is working on it.

[pohly]

@NikhilSharmaWe: you can continue with test/e2e/storage in #105860.

[NikhilSharmaWe]

@pohly what is to be changed in this case

[NikhilSharmaWe]

@chetak123 can you inform on which directory you are working.

[chetak123]

Currently working on apimachinery