Use require a lot more than assert in js tests #2564
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This PR will be squashed upon merging. I have separate commits as that was easier to work with locally. You can review them all at the same time, but I would recommend disabling whitespace diffing ;) |
na--
reviewed
Jun 13, 2022
|
|
||
| f, err := os.Open(logFilename) | ||
| assert.NoError(t, err) | ||
| f, err = os.Open(logFilename) //nolint:gosec |
There was a problem hiding this comment.
This is weird 😕 getSimpleRunner() uses afero.NewMemMapFs() by default, so why (and how) are we polluting the real filesystem here? 😕
There was a problem hiding this comment.
We don't use afero in the console code at all - I guess this is a thing we can open a issue about 🤔
Line 44 in aa06414
There was a problem hiding this comment.
hmm... 🤔 yeah, please open an issue, we should fix this 😞
na--
approved these changes
Jun 13, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The js tests had a bunch of places where they were using
assertinstead ofrequire.Mostly cases where the test should've not continued such as expecting error or no error. In some places
if assert....was used which also is completely not needed.I have kept most usage of
assertwhere they will report multiple (and even singular failures sometimes), but I would argue in most cases just usingrequirefor everything is likely enough.There are also a few places where I've added
requiresthat were previously missing.Finally, there are few places that could've been left as
assertbut I decided to go forrequirein order for there to be noassert.Errorfor example. As I would argue this should always be with require.Some of those lead to fewer reported failures as previously an
if assert ...was followed by more code that could run even if that assert is a failure. But I would argue in all of those cases that is unlikely, and even if that happens this particular failures will likely not help debug the original problem.