Fix: Jasmine reports toEqual(0, Number.MIN_VALUE) to be true

[dubzzz]

Description

The current implementation of toEqual returns an invalid answer for:

toEqual(0, Number.MIN_VALUE)
// expected to be false but was true before the fix

Similar to a bug found in Jest jestjs/jest#7941

Motivation and Context

This bug has been discovered when running property based testing test against Jest. Then I discovered the very same code was part of many other libs including Jasmine.

Please note that following this bug report on Jest, property based tests are now used within Jest to avoid future issues.

How Has This Been Tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[sgravrock]

Could you elaborate on how you found this particular case? I tried running a port of the Jest test that you linked to against the revision of Jasmine just before your fix. It correctly failed, but only if I explicitly included [0, 5e-324] as an example like the Jest test does. It looks like 100 combinations of parameters are tried, which suggests that the odds of hitting [0, 5e-324] randomly are pretty low.

I can definitely see the value in adding property tests to Jasmine's test suite, but I want to make sure I'm doing it right.

Here's the test that I ran:

  var anythingSettings = {
    key: fc.oneof(fc.string(), fc.constantFrom('k1', 'k2', 'k3')),
    maxDepth: 2, // Limit object depth (default: 2)
    maxKeys: 5, // Limit number of keys per object (default: 5)
    withBoxedValues: true,
    withMap: false,
    withSet: false
  };

  it('should be symmetric', function() {
    fc.assert(
      fc.property(
        fc.anything(anythingSettings),
        fc.anything(anythingSettings),
        function(a, b) {
          return jasmineUnderTest.matchersUtil.equals(a, b) === jasmineUnderTest.matchersUtil.equals(b, a);
        }
      ),
      {
        // Uncommenting this will produce a failure.
        // examples: [
        //   [0, 5e-324]
        // ]
      }
    );
  });

[dubzzz]

Detection of toEqual bug

Indeed the probability to detect this particular case in a hundred runs is pretty low knowing the scope of inputs we have to cover (any objects ranging from number to deep-object). In normal scenario, property based testing performs quite well on a hundred cases. But finding a bug in Jasmine is not so trivial.

For instance, in order to detect this bug in Jest (and some of the others jestjs/jest#7975 or jestjs/jest#7937), I artificially increased the number of runs locally to simulate multiple CI executions by adding { numRuns: 1000000 } in fc.assert(property, { numRuns: 1000000 }).

Your implementation

Looks good to me ;)

You may want to add withSet: true, withMap: true and withObjectString: true to detect other problems. I also reported an issue related to Set and Map in Jest using this very same test.

Property based philosophy

In general when I introduce property based testing to others I often state the following:

Today our tests are made of manually hardcoded scenarii that do not fully cover the range of all possible inputs of our functions - eg.: expect(add(1, 2)).toEqual(3). They often fails to be 100% bulletproof.

In order to cover more we need to introduce random. But random in a reproducible way (with a seed) and not deterministic because we want to try new scenarii at each CI runs (see google/oss-fuzz which shows the benefits of fuzz approach).

Additionally contrary to a naive random generator, property based testing frameworks are able to reduce the bugs to smaller cases easier to troubleshoot for a human. Instead for reporting a crash on a one-million characters string it will report a smaller string with the minimal amount of data required to reproduce the bug.

Please let me know if you need help to add property based to Jasmine

I'm glad to see that you are considering the possibility to add property based into your test suites.

[dubzzz]

@sgravrock Did you reproduce the issue?

[sgravrock]

Yes, I did. By setting numRuns: 10000 I was able to get the test to fail consistently. That takes about 3.5 seconds on my machine. So I'm thinking about using the default number of runs in local development and something much higher on Travis. That way we should be able to keep the TDD loop fast (Jasmine's entire test suite runs in about 1.5s on my machine) while also getting better coverage.