Draft: list 'pending' tests #1596

ace-n · 2017-11-22T06:21:28Z

Fixes #583.

N.B: I haven't added any new tests yet - but I plan to once the implementation details are nailed down.

Example test.js

const test = require(`ava`);

test('instant', (t) => {
	t.pass()
})

test.cb('2 seconds', (t) => {
	setTimeout(() => {
		t.pass()
		t.end()
	}, 2000);
})

test.cb('6 seconds', (t) => {
	setTimeout(() => {
		t.pass()
		t.end()
	}, 6000);
})

test.cb('12 seconds', (t) => {
	setTimeout(() => {
		t.pass()
		t.end()
	}, 12000);
})

$ ava test.js -T 3s
  2 passed
  2 pending

  These tests did not finish:

   6 seconds

   12 seconds

  1 exception

  ✖ Exited because no new tests completed within the last 3000ms of inactivity

novemberborn · 2017-11-28T17:42:40Z

Nice work @ace-n! Test state jumps through a lot of hoops on its way to the main process so well done following it all.

I'd like to suggest a slightly different approach though, one that is a bit more explicit and requires fewer low-level changes. See the inline comments below. Let me know what you think!

novemberborn · 2017-11-28T17:13:54Z

lib/runner.js

@@ -146,17 +147,22 @@ class Runner extends EventEmitter {
 	}

 	buildStats() {
+		this.tests.finalize();


How does this work given that TestCollection#finalize() does the finalizing via setImmediate()?

I think it'd be better to get an array of pending tests from TestCollection and then iterate through it, calling addTestResult(). I'd then modify addTestResult() to take a second pending argument and provide the correct value here and in run() below. That way Test doesn't have to be aware of pending at all.

TestCollection#finalize() can then be removed. For clarity I think buildStats() should be renamed to exit().

Agreed and done - though this does burrow a bit into the TestCollection, I feel like adding such a loop at the top of buildStats() (or exit(), if we were to rename it) is a cleaner way to do this.

I feel like adding such a loop at the top of buildStats() (or exit(), if we were to rename it) is a cleaner way to do this.

Which is what you've done, right?

novemberborn · 2017-11-28T17:15:50Z

lib/run-status.js

 		this.skipCount = sum(this.stats, 'skipCount');
 		this.todoCount = sum(this.stats, 'todoCount');
 		this.failCount = sum(this.stats, 'failCount');
-		this.remainingCount = this.testCount - this.passCount - this.failCount - this.skipCount - this.todoCount - this.knownFailureCount;
+		this.remainingCount = this.testCount - this.passCount - this.failCount - this.skipCount - this.todoCount - this.knownFailureCount - this.pendingCount;


At first blush you'd think this is now always 0, but that wouldn't be the case with --fail-fast not emitting the pending tests. Perhaps a comment would help clarify that possibility.

See follow-up above.

novemberborn · 2017-11-28T17:16:46Z

lib/reporters/mini.js

+				const title = test.title;
+				status += '\n   ' + colors.title(title) + '\n';
+				// TODO: Output description with link
+				// status += colors.stack(description);


This doesn't apply to pending tests.

novemberborn · 2017-11-28T17:19:42Z

lib/cli.js

@@ -207,4 +207,7 @@ exports.run = () => {
 				});
 			});
 	}
+
+	// Trap SIGINT to prevent AVA quitting immediately
+	process.on('SIGINT', () => {});


This relies on the signal still being propagated to the worker processes, right? I think we need to take a more explicit approach. For one the API needs to know not to start new workers when concurrency is being limited. Since the API is managing the worker pool it could shut down the workers, and the workers could then emit their pending tests. See https://github.com/avajs/ava/blob/e401bd151b1a81f79ccb106f034b2e18715ee125/api.js#L245:L247

(Though looking at that code I have a suspicion timeouts don't prevent new workers from being started!)

We'd need to make sure the signal isn't propagated, or otherwise is ignored in the workers.

The teardown process is initiated by the SIGINT handler added to lib/main.js - which then calls the exit() function in that same file (which I'm relying on to handle any required tear-down tasks).

If you disable the SIGINT handler in lib/main.js while keeping this one, AVA (from what I can see through the CLI) basically ignores SIGINTs. If you disable this SIGINT handler however, lib/main.js doesn't have enough time to gracefully exit.

This was largely based on experimenting with the code though - so it's definitely possible that I'm initiating the wrong teardown call/event.

I'd like to make sure we try intercepting SIGINT in the main process, and then using the Api instance to explicitly exit all workers. We may then need to trap SIGINT in the workers to ensure they don't exit before they're told to do so.

Done - any files that haven't started running when a SIGINT is received will trigger an AvaError stating that they weren't run. I assume that works?

novemberborn · 2017-11-28T17:24:16Z

lib/reporters/verbose.js

@@ -91,6 +91,7 @@ class VerboseReporter {
 				'  ' + colors.error(runStatus.failCount, plur('test', runStatus.failCount), 'failed') :
 				'  ' + colors.pass(runStatus.passCount, plur('test', runStatus.passCount), 'passed'),
 			runStatus.knownFailureCount > 0 ? '  ' + colors.error(runStatus.knownFailureCount, plur('known failure', runStatus.knownFailureCount)) : '',
+			runStatus.pendingCount > 0 ? '  ' + colors.todo(runStatus.pendingCount, plur('test', runStatus.pendingCount), 'pending') : '',


I'm sure it's on your list, but this still needs to print the titles of these tests.

Done. I missed this originally - good catch!

novemberborn · 2017-11-28T17:29:42Z

lib/main.js

@@ -27,6 +27,9 @@ Error.stackTraceLimit = Infinity;

 function test(props) {
 	if (isFailed) {
+		if (props.pending) {
+			adapter.send('test', props);
+		}


This only applies to when --fail-fast is used. I don't think we don't need to print pending tests in that circumstance.

I imagine people want to be able to identify any non-passing tests with --fail-fast. That's doable without this PR if all tests are serial (--> any tests after the failed one are pending), but harder to reason about if test order is nondeterministic or involves multiple files executing concurrently.

Thoughts?

I imagine people want to be able to identify any non-passing tests with --fail-fast.

No, all tests should stop running as soon as a single failure occurs. See #1158.

I misspoke - sorry about that 😄 s/non-passing/pending/g

That being said - I see two distinct use cases for --fail-fast:

as a simple yes/no "Do my tests pass?" check? In this case, I'd agree that people don't really care about which tests are pending.

iterative development/debugging. Personally, I'd find knowing which tests are pending useful in this case - especially if I'm trying to troubleshoot why said tests are pending.

If Option 1 is more important, I have no objection to hiding pending tests on --fail-fast - but I'd vote to keep things the way they are if we're prioritizing Option 2.

iterative development/debugging. Personally, I'd find knowing which tests are pending useful in this case - especially if I'm trying to troubleshoot why said tests are pending.

It would show tests that haven't yet completed when another test fails. I don't think that's what we're trying to solve with this PR.

sindresorhus · 2017-11-28T19:59:13Z

lib/test-collection.js

+
+	finalize() {
+		globals.setImmediate(() => {
+			Array.from(this.pendingTestInstances).forEach(test => {


Use a for-of loop instead of forEach.

novemberborn · 2017-12-01T17:11:07Z

lib/runner.js

+				result: test
+			});
+		}
+		this.tests.pendingTestInstances.clear(); // Pending tests must be cleared


Why must they be cleared?

There are spurious repeated results in some tests otherwise - namely here. This is a bit of a band-aid fix though, so let me know if further investigation or a more proper fix is required.

novemberborn · 2017-12-01T17:12:53Z

lib/cli.js

@@ -207,4 +207,7 @@ exports.run = () => {
 				});
 			});
 	}
+
+	// Trap SIGINT to prevent AVA quitting immediately
+	process.on('SIGINT', () => {});


I'd like to make sure we try intercepting SIGINT in the main process, and then using the Api instance to explicitly exit all workers. We may then need to trap SIGINT in the workers to ensure they don't exit before they're told to do so.

ace-n

Another side-question: do we want to report pending after() calls? e.g. if all your tests pass, but AVA doesn't terminate automatically - should we tell users which after() call is pending?

Perhaps this should be part of another issue altogether? I've personally ran into this in the past, but I don't know whether this feature would be useful for others.

ace-n · 2017-12-31T03:39:09Z

lib/main.js

@@ -27,6 +27,9 @@ Error.stackTraceLimit = Infinity;

 function test(props) {
 	if (isFailed) {
+		if (props.pending) {
+			adapter.send('test', props);
+		}


I misspoke - sorry about that 😄 s/non-passing/pending/g

That being said - I see two distinct use cases for --fail-fast:

as a simple yes/no "Do my tests pass?" check? In this case, I'd agree that people don't really care about which tests are pending.

iterative development/debugging. Personally, I'd find knowing which tests are pending useful in this case - especially if I'm trying to troubleshoot why said tests are pending.

If Option 1 is more important, I have no objection to hiding pending tests on --fail-fast - but I'd vote to keep things the way they are if we're prioritizing Option 2.

ace-n · 2017-12-31T03:42:31Z

lib/runner.js

+				result: test
+			});
+		}
+		this.tests.pendingTestInstances.clear(); // Pending tests must be cleared


There are spurious repeated results in some tests otherwise - namely here. This is a bit of a band-aid fix though, so let me know if further investigation or a more proper fix is required.

ace-n · 2017-12-31T03:51:21Z

lib/cli.js

@@ -207,4 +207,7 @@ exports.run = () => {
 				});
 			});
 	}
+
+	// Trap SIGINT to prevent AVA quitting immediately
+	process.on('SIGINT', () => {});


Done - any files that haven't started running when a SIGINT is received will trigger an AvaError stating that they weren't run. I assume that works?

novemberborn · 2018-01-04T16:13:19Z

api.js

 					const forkArgs = execArgvList[index];
+					if (interrupted) {
+						const relFile = path.relative('.', file);
+						return reject(new AvaError(`File ${relFile} was not run.`));


AVA prints this as an uncaught exception, which isn't ideal. Do you think this even needs to be printed, or can these test files be skipped silently?

novemberborn · 2018-01-04T16:16:55Z

api.js

 					const test = this._runFile(file, runStatus, forkArgs);
 					tests.push(test);

+					process.on('SIGINT', () => {


This will trigger "memory leak" warnings when there are more than 10 test files. Each test is pushed onto the tests array though so you could use that to set up one SIGINT listener and exit each active test.

novemberborn · 2018-01-04T16:20:08Z

lib/main.js

@@ -27,6 +27,9 @@ Error.stackTraceLimit = Infinity;

 function test(props) {
 	if (isFailed) {
+		if (props.pending) {
+			adapter.send('test', props);
+		}


iterative development/debugging. Personally, I'd find knowing which tests are pending useful in this case - especially if I'm trying to troubleshoot why said tests are pending.

It would show tests that haven't yet completed when another test fails. I don't think that's what we're trying to solve with this PR.

novemberborn · 2018-01-04T16:22:04Z

Another side-question: do we want to report pending after() calls? e.g. if all your tests pass, but AVA doesn't terminate automatically - should we tell users which after() call is pending?

Perhaps this should be part of another issue altogether? I've personally ran into this in the past, but I don't know whether this feature would be useful for others.

That would be useful, yes, but it could be done in a separate issue.

novemberborn · 2018-02-11T17:32:01Z

There's been a lot of changes recently that impact this PR. I'm also looking to refactor how test results are collected and logged. That'll make this feature easier to implement.

Closing this for now. @ace-n if you want to pick this up again, please check in first to see if I've actually managed to do that refactoring 👼 Meanwhile thank you for putting in so much effort!

First draft of listing pending tests

6f253e3

novemberborn requested changes Nov 28, 2017

View reviewed changes

sindresorhus reviewed Nov 28, 2017

View reviewed changes

Address PR comments

3abe748

novemberborn reviewed Dec 1, 2017

View reviewed changes

Address second round of PR comments

ccf89fc

ace-n commented Dec 31, 2017

View reviewed changes

Remove stray space

0f5a16a

novemberborn requested changes Jan 4, 2018

View reviewed changes

novemberborn closed this Feb 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: list 'pending' tests #1596

Draft: list 'pending' tests #1596

ace-n commented Nov 22, 2017

novemberborn commented Nov 28, 2017

novemberborn Nov 28, 2017

ace-n Dec 1, 2017 •

edited

Loading

novemberborn Dec 1, 2017

novemberborn Nov 28, 2017

ace-n Dec 1, 2017 •

edited

Loading

novemberborn Nov 28, 2017

ace-n Dec 1, 2017

novemberborn Nov 28, 2017

ace-n Dec 1, 2017 •

edited

Loading

novemberborn Dec 1, 2017

ace-n Dec 31, 2017

novemberborn Nov 28, 2017

ace-n Dec 1, 2017

novemberborn Nov 28, 2017

ace-n Dec 1, 2017

novemberborn Dec 1, 2017

ace-n Dec 31, 2017

novemberborn Jan 4, 2018

sindresorhus Nov 28, 2017

ace-n Dec 1, 2017

novemberborn Dec 1, 2017 •

edited

Loading

ace-n Dec 31, 2017

novemberborn Dec 1, 2017

ace-n left a comment

ace-n Dec 31, 2017

ace-n Dec 31, 2017

ace-n Dec 31, 2017

novemberborn Jan 4, 2018

novemberborn Jan 4, 2018

novemberborn Jan 4, 2018

novemberborn commented Jan 4, 2018

novemberborn commented Feb 11, 2018

Draft: list 'pending' tests #1596

Draft: list 'pending' tests #1596

Conversation

ace-n commented Nov 22, 2017

novemberborn commented Nov 28, 2017

Choose a reason for hiding this comment

ace-n Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ace-n Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ace-n Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novemberborn Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ace-n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novemberborn commented Jan 4, 2018

novemberborn commented Feb 11, 2018

ace-n Dec 1, 2017 •

edited

Loading

ace-n Dec 1, 2017 •

edited

Loading

ace-n Dec 1, 2017 •

edited

Loading

novemberborn Dec 1, 2017 •

edited

Loading