-
-
Notifications
You must be signed in to change notification settings - Fork 79k
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
Remove potential false positive assertions #33288
Remove potential false positive assertions #33288
Conversation
Can you check if there are more cases please? |
ab08462
to
14ae0ce
Compare
Added more cases I could find in unit tests for assertions against values that are assigned from |
Thanks for the PR. There must be more instances, from a quick look. At least for |
Added in ee7e291 👍 |
Do we still want these changes? I can rebase to resolve conflicts so it can be ready to merge, otherwise I will close this PR |
@twbs/js-review this might has to be in 5-stable |
Thanks @GeoSot please let me know when is the best time to rebase, it would be great if I can avoid doing it multiple times 👍 |
in case of stable-5 is a good time to be rebased. But it doesn't depends by me and only. Thanks for your understanding |
querySelector() returns null but expect(document.querySelector('...')).toBeDefined() tests that the value is not undefined
…stance() calls in tests
…or .getInstance() call
ee7e291
to
bc8be0e
Compare
I have rebased the changes against master |
Sorry for the delay @spicalous but we are only a few volunteers. This PR can wait after 5.0.0, since it doesn't fix any bug. I hope you understand. The same applies to your other PR you closed, you should reopen it until someone has a look at it. |
No problemos, just give me a shout when this needs to be rebased again 👍 |
@spicalous I saw about 17 more |
When looking through some of the matchers, I think it's not very clear when some return null or undefined What do you guys think about using |
toBeTruthy() coerces the values AFAICT so it's a little moot IMHO. Either we are always explicit or not :) |
@GeoSot I have updated more cases Regarding these ones, I think they provide no value and can be removed. What do you guys think?
bootstrap/js/tests/unit/scrollspy.spec.js Line 79 in f61a021
|
So the original reason for this PR was that I noticed the assertions could result in a failing test to pass e.g. in the case where element is null Similar case for truthy checks Given that a property is null, we assert In these cases, wouldnt truthy and falsy assertions be better? Ah... Javascript..... :) |
querySelector() returns null but
expect(document.querySelector('...')).toBeDefined()
checks that the value is notundefined
. This can lead to false positive tests