[testharness.js] remove superfluous test.done() call

[foolip]

No description provided.

[foolip]

@jugglinmike what sort of things should I watch out for when judging if this will regress anything? If it were unambiguously dead code I imagine you would've already removed it...

[jugglinmike]

@jugglinmike what sort of things should I watch out for when judging if this will regress anything?

We can have a high degree of confidence in this change because the relevant code is very close:

 if (tests.file_is_test) {
     var test = tests.tests[0];
     if (test.phase >= test.phases.HAS_RESULT) {
         return;
     }
     test.set_status(test.FAIL, e.message, stack);
     test.phase = test.phases.HAS_RESULT;
-    // The following function invocation is superfluous.
-    // TODO: Remove.
-    test.done();
 } else if (!tests.allow_uncaught_exception) {
     tests.status.status = tests.status.ERROR;
     tests.status.message = e.message;
     tests.status.stack = stack;
 }
 done();

And the definition of done:

function done() {
    if (tests.tests.length === 0) {
        tests.set_file_is_test();
    }
    if (tests.file_is_test) {
        // file is test files never have asynchronous cleanup logic,
        // meaning the fully-synchronous `done` function can be used here.
        tests.tests[0].done();
    }
    tests.end_wait();
}

If it were unambiguously dead code I imagine you would've already removed it...

I didn't remove it immediately because I discovered it in the context of a very involved patch and wanted to avoid superfluous change. I didn't remove it following that because I simply forgot.

[foolip]

The done you linked is the global done, not the one that this call would invoke. (On phone now and can't check what the other one does.)

[jugglinmike]

Agreed. It's not that test.done() calls the global done. It's that the global done calls tests.tests[0].done(). Since the error handler calls the global done unconditionally, we can be confident that removing tests.tests[0].done() from the error handler will not change any behavior.

Put in drastically oversimplified terms, the removal looks like this:

 function handle_error() {
-  tests.tests[0].done();
   done();
 }
 
 function done() {
   tests.tests[0].done();
 }

[foolip]

Thanks Mike, with that I feel I need not put this through a full CI run.