Opened 10 years ago

Closed 7 years ago

#8033 closed defect (invalid)

doh.runner: using multiple doh.Deferred objects in a callback chain will cause a failed test to reported as "passed".

Reported by: Snowman Owned by: dylan
Priority: high Milestone: future
Component: TestFramework Version: 1.2.0
Keywords: doh runner deferred callback chain pass fail Cc:
Blocked By: Blocking:

Description

The attached sample explains it best.

All 3 tests fail, as they should, and the test summary reports all 3 failures, but in the log test 3 is reported as a PASS.

The problem appears to be in the method doh._runFixture, where it calls doh._testFinished (the first time). It makes the following call:

doh._testFinished(groupName, fixture, ret.results[0]);

The third parameter is supposed to be a boolean value indicating success or failure, however when you are using chained callbacks with multiple Deferred objects (as in the example), the value ret.results[0] can contain a non-empty value even if the test ultimately failed. I recommend replacing the line with this:

doh._testFinished(groupName, fixture, ret.fired === 0);

The "fired" attribute seems to be the best indicator of success or failure, not "results[0]".

Attachments (1)

dohrunnerbug.html (1.6 KB) - added by Snowman 10 years ago.

Download all attachments as: .zip

Change History (6)

Changed 10 years ago by Snowman

Attachment: dohrunnerbug.html added

comment:1 Changed 10 years ago by Snowman

I've now encountered a case where my proposed fix doesn't work either (ie. a test fails, but ret.fired is 0). I haven't got time to investigate or to produce another test case, but I suspect that the only reliable way to determine whether a test succeeded when it uses a doh.Deferred object is to add an "errback" handler. My current (somewhat hacked) solution is to use the following code in doh._runFixture:

var success = true;
ret.addErrback(function(err){
	success = false;
	doh._handleFailure(groupName, fixture, err);
});

var retEnd = function(){
	if(fixture["tearDown"]){ fixture.tearDown(doh); }
	tg.inFlight--;
	if((!tg.inFlight)&&(tg.iterated)){
		doh._groupFinished(groupName, (!tg.failures));
	}
	doh._testFinished(groupName, fixture, success);
	if(doh._paused){
		doh.run();
	}
}

comment:2 Changed 10 years ago by dante

Milestone: tbdfuture

comment:3 Changed 8 years ago by Chris Mitchell

Owner: changed from alex to dylan

please review/triage

comment:4 Changed 7 years ago by mm

I have just run across the similar problem in 1.7 runner.js line 1328

doh._testFinished(groupName, fixture, ret.results[0]);

ret.results is undefined.

I will file a new bug however

comment:5 Changed 7 years ago by bill

Resolution: invalid
Status: newclosed

The third test case from the attached test file just looks like a user error. It's:

function test3(){
	var deferred = new doh.Deferred();
	deferred.addCallback(function(){
		var deferred2 = new doh.Deferred();
		deferred2.addCallback(function(){
			doh.assertTrue(false);
		});
		setTimeout(function(){
			deferred2.callback();
		}, 0);
		return deferred2;
	});
	setTimeout(function(){
		deferred.callback();
	}, 0);
	return deferred;
}

Since you return deferred, and the test calls deferred.callback(), it's right for the test to pass. Maybe you meant to return defered2, but you are also calling deferred2.callback(), so even returning deferred2 would pass. What you really need is an errback() call somewhere, or better yet a deferred.getTestErrback() and deferred2.getTestCallback() calls.

Note: See TracTickets for help on using tickets.