Remove potential false positive assertions

[spicalous]

querySelector() returns null but expect(document.querySelector('...')).toBeDefined() checks that the value is not undefined. This can lead to false positive tests

[XhmikosR]

Can you check if there are more cases please?

[spicalous]

Added more cases I could find in unit tests for assertions against values that are assigned from .querySelector(...)

[XhmikosR]

Thanks for the PR. There must be more instances, from a quick look. At least for getInstance.

[spicalous]

Added in ee7e291 👍

[spicalous]

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

[GeoSot]

@twbs/js-review this might has to be in 5-stable

[spicalous]

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 👍

[GeoSot]

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

[spicalous]

I have rebased the changes against master

[XhmikosR]

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.

[spicalous]

No problemos, just give me a shout when this needs to be rebased again 👍

[GeoSot]

@spicalous I saw about 17 more deDefined checks. If it is easy, please make a check on them too

[spicalous]

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 / toBeFalsy ?

[XhmikosR]

toBeTruthy() coerces the values AFAICT so it's a little moot IMHO. Either we are always explicit or not :)

[spicalous]

@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/dom/manipulator.spec.js

Line 118 in f61a021

expect(offset).toBeDefined()

bootstrap/js/tests/unit/dom/manipulator.spec.js

Line 131 in f61a021

expect(position).toBeDefined()

bootstrap/js/tests/unit/scrollspy.spec.js

Line 79 in f61a021

expect(scrollSpy).toBeDefined()

[spicalous]

toBeTruthy() coerces the values AFAICT so it's a little moot IMHO. Either we are always explicit or not :)

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
expect(element).toBeDefined() === TEST PASS false positive
expect(element).not.toBeNull() === TEST FAIL correct
expect(element).toBeFalsy() === TEST FAIL correct

Similar case for truthy checks

Given that a property is null, we assert .not.toBeNulll (null !== actual)
What happens if behaviour is changed
e.g.
instead of property = null, someone may delete property
In this case, the assertion would pass as undefined !== null and we end up with false positives again

In these cases, wouldnt truthy and falsy assertions be better?

Ah... Javascript..... :)