Replace assert with require in e2e channel defaults webhook test

[xtreme-sameer-vohra]

Problem
use "require.Nil" instead of "assert.Equal" for test setup. The latter
notes a failure but doesn't fail immediately. This resulted in panics
in subsequent sections of the test and made it hard to pin point the
reason. "require.Nil" will output the error and fail immediately
making it easier to debug issues with test setup.

Channel Defaults Webhook Test
Specifically, at least these locations (there might be others uses of assert.Nil that fall under test setup rather than test assertion and therefore should use require.Nil)

• L136
• L140
• L167

Persona:
Which persona is this feature for?
Contributor

Additional context (optional)
Original Issue & PR for similar broker test: #4717, #4720

[xtreme-sameer-vohra]

I'd appreciate some feedback regarding:
This can be a good first issue or the scope could be expanded to audit other e2e tests so that the usage is more clearer and consistent across the suite.

[zhongduo]

I think "not failing immediately" might be useful in unit tests due to table testing. But I cannot think of any reason for e2e test, so I would vote for failing early.

[xtreme-sameer-vohra]

Thanks and is it better do have an issue/test or issue/e2e suite? The former makes it more suitable for folks getting started and doable in tiny quick chunks but adds overhead of many issues/prs 🤷

[zhongduo]

Not sure if I understand the concern, I would expect this to be in one PR. Previously I made the webhook fix a PR because that's a bug fix and I don't want to be delayed. But I do think all the changes can be in one PR.

…

On Wed, Jan 13, 2021 at 8:55 PM Sameer Vohra ***@***.***> wrote: Thanks and is it better do have an issue/test or issue/e2e suite? The former makes it more suitable for folks getting started and doable in tiny quick chunks but adds overhead of many issues/prs 🤷 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4739 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACE6CNE44DYW2J3WL72UBPDSZZFHTANCNFSM4WBT34XA> .

[xtreme-sameer-vohra]

Sorry, I wasn't clear. What I was trying to get some guidance on was for a cross cutting change like this is it better to

• have one issue per test file eg. channel_defaults_webhook_test.go and then the PR can close it out
• have one issue for the entire suite e2e suite

Option 1 makes it good for new contributors as the changes are minor but there is the extra overhead of many issues & PRs to span the suite
Option 2 makes it harder for new contributors but there is less overhead as 1 issue & PR solve the problem

For now, I'll assume having an issue per test file is the way to go.

[zhongduo]

IMO option 2 makes more sense.

…

On Thu, Jan 14, 2021 at 1:07 PM Sameer Vohra ***@***.***> wrote: Sorry, I wasn't clear. What I was trying to get some guidance on was for a cross cutting change like this is it better to - have one issue per test file eg. channel_defaults_webhook_test.go and then the PR can close it out - have one issue for the entire suite e2e suite Option 1 makes it good for new contributors as the changes are minor but there is the extra overhead of many issues & PRs to span the suite Option 2 makes it harder for new contributors but there is less overhead as 1 issue & PR solve the problem For now, I'll assume having an issue per test file is the way to go. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4739 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACE6CND5JLS4KNROW2ZQRULSZ4XH3ANCNFSM4WBT34XA> .

[lberk]

@xtreme-sameer-vohra it might be worth adding a checklist of all the places we need to make this replacement to this issue, and check them off as they're fixed. Thanks!

[xtreme-sameer-vohra]

@xtreme-sameer-vohra it might be worth adding a checklist of all the places we need to make this replacement to this issue, and check them off as they're fixed. Thanks!

done. Since this hasn't been picked up yet, I'll take it.

/assign