have Base generate the strings for error messages immediately

[abrady0]

instead of delaying until epilogue

Requirements

Description of the Change

An object referred to by a test failure can be mutated between the point of test failing and when the object is converted to a string and output. For instance if an afterEach clears the fields in an object pointed to by a chai.expect.eql AssertionError's expected or actual fields then the reporter's error message will be misleading

see https://codepen.io/abrady0/pen/pWGBxR?editors=1111 for running example.

Here is a snippet that assumes 'spec' reporter:

describe('example', function() {
  var foo = {
    a: 'before'
  };

  afterEach(function() {
    console.log('afterEach');
    foo.a = 'after';
  })
  it('should print the error properly', function() {
    var bar = { a: 'during' };
    console.log('foo2')
    // HERE: output *should* say 'before' instead of 'after' for the previous line
    //  + expected - actual
    //   {
    //  -  'a': 'after'
    //  +  'a': 'during'
    //   }
    // 
    //  + expected - actual
    //   {
    //  -  'a': 'before'
    //  +  'a': 'during'
    //   }
    expect(foo).to.deep.equal(bar);
  });  
});

Alternate Designs

To fix this in chai would require either cloning or stringifying the actual and expected objects at the time of error creation.

Cloning the objects is a potentially expensive, and not always possible operation that would put new constraints on what could be used in an eql comparison.

Stringifying is a possibility as that is an existing requirement for using the Base reporter, however mocha has its own stringifying function with its own configuration and behavior (such as sorting fields in an object) and would require that chai, and any other libraries similar to chai, import mocha and use mocha.utils.stringify (or perhaps another stringifier depending on the reporter), which feels like it breaks modularity pretty hard

Even if this was fixed in the chai module, other modules would have to take on the burden of making sure their actual and expected objects were in mocha compatible string form.

Why should this be in core?

This functionality directly affects the Base reporter.

Benefits

The benefit realized by this code change is correct error messages in situations where objects that can be mutated after changing are passed to the Base reporter

Possible Drawbacks

Code that would have executed later is now run when a test fails. This puts an extra burden on memory requirements at the time of test failure and could possibly result in running out of memory space in a tightly constrained test when it fails.

The cost of execution should be the same, but any time you change when things happen it risks having side effects. It is possible the configuration of the reporter is changed in some way that would make the output different before and after this change.

Possibly the string representation of an object is larger than its non-string representation and causes too much memory to be used over the lifetime of the test.

Unlikely but there might be side effects from having the expected and actual no longer referenced such that they get garbage collected at a different time (and maybe are referenced by a weakmap? I'm reaching here)

It is possible this bug will appear again in new reporters depending on if they build on the Base reporter or not.

Applicable issues

I didn't make an issue for the fix, just noticed it today in a test and thought I'd take a shot at fixing it.

This is just a bug fix

[jsf-clabot]

All committers have signed the CLA.

[coveralls]

Coverage increased (+0.05%) to 89.86% when pulling 760b4db on abrady0:master into da901da on mochajs:master.

[boneskull]

@abrady0 Thanks for the detailed explanation. Hopefully you can entertain my questions...

1. Do you have a non-contrived use case where this is a problem? It's unusual to do something like the example.
2. Can you speak to Object.freeze() as a potential solution?

I may have more comments/questions later, but this is just top of mind.

[abrady0]

Hey @boneskull!

This popped up in one of our unit tests that was exercising some code that accumulated responses from several different async sources and then processed it. for the suite we initialized a single context to be used by multiple tests (as context initialization has some cost), and simulated the responses to test the handling, clearing the accumulator after each test.

Object.freeze is potentially problematic because it could break any existing tests that rely on mutating objects after a test fails, and maybe also because if we want the state of the object to match exactly it would have to be a recursive freeze which could be time consuming if the object was especially large. (I think the first reason is the more legit one, if you're already doing a huge compare the cost of freezing recursively on fail probably won't be a factor).

Agree that this is unusual, perhaps suggesting that the freeze approach is better? I'm open either way.

Sorry for the late reply!

[boneskull]

I'm going to have to take a closer look at this. It seems OK, but in my experience, those are always the ones that bite me in the ass. 😉

[boneskull]

I can find no obvious slowdown or extra memory pressure when run against Babel (Mocha, Chai) nor TypeScript (just Mocha), or Chai itself.

LGTM

[coveralls]

Coverage increased (+0.05%) to 89.905% when pulling 24b65e8 on abrady0:master into 709a09e on mochajs:master.