Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add test with leading plan #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

Krinkle
Copy link

@Krinkle Krinkle commented Jul 7, 2024

Upon reviewing this for potential adoption, I was confused by the following code:

function check() {
	// ..
	if (seen.plan === null || seen.asserts.length < seen.plan) { return; }
	finish();
}

// …

p.on('assert', function (result) {
	seen.asserts.push(result);
	check();
});

p.on('plan', function (plan) {
	seen.plan = plan.end - plan.start;
});

If my understanding of the TAP spec is correct, a plan like 1..4 represents 4 tests from test 1 to test 4. The above appears to have an off-by-one error where it stores 4 - 1 = 3, and the check ends as soon as length == 3, i.e. after test 3 instead of after test 4.

I noticed there was no test case with a leading plan yet, and writing one, it appears to fail. But, perhas I'm misunderstanding how this is supposed to work. In case this is a bug, here's what I found so far.

$ node test/leading.js 
TAP version 13
# (anonymous)
## [test] write: TAP version 13
## [test] write: 1..3
## [src] plan event { start: 1, end: 3 } 2
## [test] write: ok 1 first
## [test] write: ok 2 what
## [src] assert event 1
## [test] write: ok 3 third base
## [src] assert event 2
## [src] check() calls finish()
## [src] finish() calls p.end()
## [src] complete event
ok 1 should be strictly equal
not ok 2 results object
  ---
    operator: deepLooseEqual
    expected: |-
      { ok: true, count: 3, pass: 3, fail: 0, bailout: false, todo: 0, skip: 0, plan: { start: 1, end: 3, skipAll: false, skipReason: '', comment: '' }, failures: [], asserts: [ { ok: true, id: 1, name: 'first' }, { ok: true, id: 2, name: 'second' }, { ok: true, id: 2, name: 'third base' } ] }
    actual: |-
      { ok: false, count: 2, pass: 2, fail: 1, bailout: false, todo: 0, skip: 0, plan: FinalPlan { start: 1, end: 3, skipAll: false, skipReason: '', comment: '' }, failures: [ { tapError: 'incorrect number of tests' } ], asserts: [ Result { ok: true, id: 1, name: 'first' }, Result { ok: true, id: 2, name: 'what' } ] }
    at: snip
  ...
## [test] write: 
## [test] write: # weeeee
## [test] write: undefined

I added a return; to the test code where it writes the last undefined, but that made no difference (treated as ignored extra). Adding +1 to seen.plan seems like the obvious fix, but produces a different error instead. I assumed that the reason assert event 3 was never emitted by the parser is that after event 2 the stream is closed by calling p.end(). However with the fix applied, the event still isn't emitted. It seems tap-parser doesn't want to emit it yet until the next test or a test plan appears. I added an empty line and a comment at the end but it still won't emit it.

$ node test/leading.js 
TAP version 13
# (anonymous)
## [test] write: TAP version 13
## [test] write: 1..3
## [src] plan event { start: 1, end: 3 } 3
## [test] write: ok 1 first
## [test] write: ok 2 what
## [src] assert event 1
## [test] write: ok 3 third base
## [src] assert event 2
## [test] write: 
## [test] write: # weeeee
not ok 1 plan != count
  ---
    operator: fail
    expected: 2
    actual:   0
    at: process.<anonymous> (/Users/krinkle/Temp/tap-finished/node_modules/tape/index.js:169:8)
    stack: |-
      Error: plan != count
          at Test.assert [as _assert] (/Users/krinkle/Temp/tap-finished/node_modules/tape/lib/test.js:492:48)
          at Test.fail (/Users/krinkle/Temp/tap-finished/node_modules/tape/lib/test.js:600:7)
          at Test._exit (/Users/krinkle/Temp/tap-finished/node_modules/tape/lib/test.js:443:8)
          at process.<anonymous> (/Users/krinkle/Temp/tap-finished/node_modules/tape/index.js:169:8)
          at process.emit (node:events:515:28)
  ...

1..1
Debug diff
diff --git a/index.js b/index.js
index 4fea2698bf..67d5c8af7b 100644
--- a/index.js
+++ b/index.js
@@ -23,29 +23,39 @@ module.exports = function (opts, cb) {
 			cb(assign({}, finalResult, { asserts: seen.asserts }));
 		});
 		if (opts.wait && !ended) {
+			console.info('## [src] finish() calls setTimeout()');
 			setTimeout(function () { p.end(); }, opts.wait);
-		} else { p.end(); }
+		} else {
+			console.info('## [src] finish() calls p.end()');
+			p.end();
+		}
 	}
 
 	function check() {
 		if (finished) { return; }
 		if (seen.plan === null || seen.asserts.length < seen.plan) { return; }
+		console.info('## [src] check() calls finish()');
 		finish();
 	}
 
-	p.on('end', function () { ended = true; });
+	p.on('end', function () {
+		console.info('## [src] end event');
+		ended = true;
+	});
 
 	p.on('assert', function (result) {
-		seen.asserts.push(result);
+		console.info('## [src] assert event', seen.asserts.push(result));
 		check();
 	});
 
 	p.on('plan', function (plan) {
-		seen.plan = plan.end - plan.start;
+		seen.plan = plan.end - (plan.start - 1);
+		console.info('## [src] plan event', plan, seen.plan);
 		check();
 	});
 
 	p.on('complete', function () {
+		console.info('## [src] complete event');
 		if (finished) { return; }
 		finish();
 	});
diff --git a/test/all_ok.js b/test/all_ok.js
index 93641d8025..7da1e86407 100644
--- a/test/all_ok.js
+++ b/test/all_ok.js
@@ -46,6 +46,7 @@ test(function (t) {
 		if (lines.length === 0) {
 			clearInterval(iv);
 			done = true;
+			return;
 		}
 
 		var line = lines.shift();
diff --git a/test/excess.js b/test/excess.js
index fcf1f2406a..4e15ba2ce1 100644
--- a/test/excess.js
+++ b/test/excess.js
@@ -49,6 +49,7 @@ test(function (t) {
 		if (lines.length === 0) {
 			clearInterval(iv);
 			done = true;
+			return;
 		}
 
 		var line = lines.shift();
diff --git a/test/leading.js b/test/leading.js
index 2eb092bbd6..f0700196de 100644
--- a/test/leading.js
+++ b/test/leading.js
@@ -51,7 +51,7 @@ test(function (t) {
 		}
 
 		var line = lines.shift();
-		console.log('# write: ' + line);
+		console.log('## [test] write: ' + line);
 		stream.write(line + '\n');
 	}, 25);
 });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant