Bridge's testEnd event handler is passing too much to sendMessage: JSON errors.

[jdatskuid]

The testEnd handler in the chrome bridge is passing the entire obj argument, which must be serialized into JSON. But sometimes this object includes objects which cannot be serialized, such as complex objects with circular references, in which case it throws an error and halts testing.

QUnit.on('testEnd', function(obj) {
    sendMessage('qunit.on.testEnd', obj);
});

A sample error:

>> Error: TypeError: Converting circular structure to JSON
>>     --> starting at object with constructor 'Foo'
>>     |     property 'bar' -> object with constructor 'Object'
>>     --- property 'foo' closes the circle
>>     at JSON.stringify (<anonymous>)
>>     at win.<computed> (<anonymous>:17:26)
>>     at sendMessage (<anonymous>:130:32)
>>     at Array.<anonymous> (<anonymous>:153:3)

Looking at the source code, it appears that there is no practical reason for sending up the entire object when only three properties are in use. I tested locally with the following and it seems to work well:

QUnit.on('testEnd', function(obj) {
    sendMessage('qunit.on.testEnd', {
        status: obj.status,
        errors: obj.errors,
        fullName: obj.fullName,
    });
});

This appears to be the current pattern for the runEnd event for the sake or removing large, unused properties:

QUnit.on('runEnd', function(obj) {
    // Re-create object to strip out large 'tests' field (deprecated).
    sendMessage('qunit.on.runEnd', {
        testCounts: obj.testCounts,
        runtime: obj.runtime,
        status: obj.status
    });
});

This bug appears to have been introduced by #189

[vtintillier]

Hello @vladikoff @Krinkle

do you agree this is an issue? It is blocking us from upgrading.

I'd be happy to create a PR, but I'd like to know your point of view on this first.

Thanks

[Krinkle]

@vtintillier Yes, that sounds good, and patch welcome!

I suggest including name and runtime as well, so that the API is complete with https://api.qunitjs.com/callbacks/QUnit.on/#the-testend-event.

By the way, I patched the repo as follows to see what other (unofficial or legacy) object keys there are:

  QUnit.on('testEnd', function(obj) {
+   console.log('testEnd obj: ' + Object.keys(obj).join(', '));
    sendMessage('qunit.on.testEnd', obj);
  });

....
testEnd obj: name, suiteName, fullName, runtime, status, errors, assertions

OK

Unlike with runEnd, there isn't a legacy tests or suites structure in the testEnd object. I have a feeling that perhaps the recursive structure in your case, is being found via the testEnd.assertions field as this structure includes expected and actual values.

But, while removing this does reduce the number of values captured (and thus improves the speed, which is great!) - we do capture testEnd.errors, and that followed a very similar expected: .., actual: .. structure. I suspect, thus, that if the failing test is one of those, you might encounter the same bug still. Could you confirm that?

In any event, this patch will still be a great performance improvement, and is a bug fix in so far that it avoids causing failures when a test is passing. However, I suspect that in the case of a failing test, it might continue to cause a confusing error about failed JSON serialisation, but it would be limited to builds that are already failing for other reasons at least.

[vtintillier]

Unlike with runEnd, there isn't a legacy tests or suites structure in the testEnd object. I have a feeling that perhaps the recursive structure in your case, is being found via the testEnd.assertions field as this structure includes expected and actual values.

Yes, this is it in my case.

But, while removing this does reduce the number of values captured (and thus improves the speed, which is great!) - we do capture testEnd.errors, and that followed a very similar expected: .., actual: .. structure. I suspect, thus, that if the failing test is one of those, you might encounter the same bug still. Could you confirm that?

I will confirm, but I think you're right here too.

In any event, this patch will still be a great performance improvement, and is a bug fix in so far that it avoids causing failures when a test is passing. However, I suspect that in the case of a failing test, it might continue to cause a confusing error about failed JSON serialisation, but it would be limited to builds that are already failing for other reasons at least.

I will see if we can catch the JSON error in that case, and use something else instead of the object JSON. Or maybe we just leave it like that, as I guess that's a corner case that such non serializable objects are used in assertions other than assert.ok? At least I think in our case we only check such objects are non null. But now that I think about it, assert.strictEqual would be reasonable too, and there we have the expected value to report in case of failure...

[vtintillier]

I have a PR nearly ready, however the link to the CLA seems to be broken at https://gruntjs.com/contributing Is there a new process to follow?

[Krinkle]

@vtintillier Good catch. For most repos, we now prompt CLA automatically after creating the PR. I'd say just push and create the PR and see what happens!

[vtintillier]

Created #195 , but no CLA prompt yet.

[vtintillier]

Hi @Krinkle when do you think this will get in a new release? Are there other changes planned first? Thanks

[Krinkle]

@vtintillier Thanks for the reminder. I wanted to get a QUnit release out first, but this is ready now. Nothing else blocking it besides time :). Done now.