Brackets: Unit tests should avoid expect().not.toBeNull() and .not.toBe(null)

[peterflynn]

These forms will pass when foo === undefined, which is almost always not what was intended (masking real failures):

expect(foo).not.toBeNull();
expect(foo).not.toBe(null);

We should replace them with one of these safer forms:

expect(foo).toBeTruthy();
expect(foo).not.toEqual(null);

Personally I prefer toBeTruthy(), but we could pick either one to standardize on.

We have 60+ instances of the unsafe form in our codebase right now, including unit tests in extensions/default.

[njx]

expect(equality).not.toBeConfusing();

[njx]

(1 failure)

[dangoor]

This seems backwards... In JavaScript, null == undefined is true but null === undefined is false. If you put this test into one of our test files, you'll find that it passes (or at least it does for me):

            it("shouldn't get confused about null and undefined", function () {
                var obj = {};
                expect(obj.bar).toBeUndefined();
                expect(obj.bar).not.toBeNull();
                expect(obj.bar).not.toBe(null);
                expect(obj.bar).toEqual(null);
            });

It seems that it's actually the .toEqual case that we need to be careful of.

[dangoor]

Ahh, I see what you're getting at. The .not cases are fraught with peril if something unexpectedly becomes undefined. It's perfectly safe to use .toBeNull when you're expecting null, but not to use .not.toBeNull if you're expecting a real value. Yeah, that's a good point.

[dangoor]

5492 appears to have taken care of this. FBNC to @peterflynn

[peterflynn]

Whoops, looks like a new reference to this snuck into DocumentManager-test after #5492 landed.

@jasonsanjose Is there any way we could make a Travis test that verifies the strings ".not.toBeNull()" ".not.toBe(null)" and don't appear anywhere in the files being submitted? Otherwise this might be a never-ending whack-a-mole. Unless there's a way to patch the Jasmine APIs to disallow those constructs...

[jasonsanjose]

We could write a crude grep grunt task to do this. It wouldn't necessarily be Travis specific, but we could run during the travis build and have it pass/fail the build.

[peterflynn]

Closing since all the usages of this are gone for now