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

Output logs for tests that remain pending when AVA exits #3125

Merged
merged 5 commits into from Nov 13, 2022

Conversation

kevo1ution
Copy link
Contributor

@kevo1ution kevo1ution commented Oct 25, 2022

Fixes #3122 , #2780

What
This PR plumbs test logs to be printed when tests timeout due to the global timeout set by ava --timeout <timeout> command.

Why
t.log is a very useful utility for debugging what happens when a test fails. When a test times out, these logs are useful for figuring out why the tests timed out.

unecessary dif

temp

add logs for pending tests

fix linting issues

undo unecessary change
@kevo1ution kevo1ution force-pushed the fix-issue-3222-log-after-timeout branch from 0ff9efc to ce2b12e Compare October 25, 2022 04:03
@kevo1ution
Copy link
Contributor Author

kevo1ution commented Oct 25, 2022

@novemberborn

This is the output with the same code snippet on #3122 . Before I attempt writing test cases for this, do you think this looks good implementation-wise and output-wise?

 % npm run test

> ava-logging-timeout-bug@1.0.0 test
> npx ava --timeout=1s --verbose


start promise that never resolves...
  
  ✘ Timed out while running tests

  1 tests were pending in test/example.js

  ◌ example where logs are not provided by ava when test time out
    ℹ {
        message: 'Very important debug statement, that developer would love to see in output after test run',
      }

  ─

  1 test remained pending after a timeout

@@ -9,6 +9,7 @@ export default class RunStatus extends Emittery {
super();

this.pendingTests = new Map();
this.pendingTestsLogReference = new Map();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

separate map from this.pendingTests because they are modified and initialized in different events with different objects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why Reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, I think pendingTestsLogs sounds better

@novemberborn novemberborn marked this pull request as ready for review November 1, 2022 09:28
@novemberborn novemberborn marked this pull request as draft November 1, 2022 09:28
@novemberborn
Copy link
Member

@kevo1ution this is on my list to look at more closely, but at first glance it seems reasonable.

@kevo1ution
Copy link
Contributor Author

@kevo1ution this is on my list to look at more closely, but at first glance it seems reasonable.

thanks I'll start adding several tests to confirm that the output works, and mark this ready for review when it's ready to be looked at

@kevo1ution kevo1ution marked this pull request as ready for review November 1, 2022 18:50
@kevo1ution
Copy link
Contributor Author

kevo1ution commented Nov 1, 2022

@novemberborn This is ready for review. It took some time to figure out how testing the testing framework works (lol), so hopefully, I did it up to your standards. Let me know if there is anything I missed

@kevo1ution kevo1ution changed the title Output logs for each test after global timeout is triggered Output logs for pending tests after global timeout or SIGINT is triggered Nov 3, 2022
@kevo1ution
Copy link
Contributor Author

@novemberborn bumping on this, let me know if there's anything else I need to do, thanks!

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Left some comments and suggestions around optional chaining.

@@ -370,8 +370,11 @@ export default class Reporter {
}

this.lineWriter.writeLine(`${testsInFile.size} tests were pending in ${this.relativeFile(file)}\n`);
const testTitleToLogs = evt.pendingTestsLogReference.get(file) ?? new Map();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const testTitleToLogs = evt.pendingTestsLogReference.get(file) ?? new Map();
const testTitleToLogs = evt.pendingTestsLogReference.get(file);

for (const title of testsInFile) {
const logs = testTitleToLogs.get(title) ?? [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

writeLogs() can handle falsey logs values. So with optional chaining and the above we can do:

Suggested change
const logs = testTitleToLogs.get(title) ?? [];
const logs = testTitleToLogs?.get(title);

@@ -9,6 +9,7 @@ export default class RunStatus extends Emittery {
super();

this.pendingTests = new Map();
this.pendingTestsLogReference = new Map();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why Reference?

if (this.pendingTestsLogReference.has(event.testFile)) {
this.pendingTestsLogReference.get(event.testFile).set(event.title, event.logs);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (this.pendingTestsLogReference.has(event.testFile)) {
this.pendingTestsLogReference.get(event.testFile).set(event.title, event.logs);
}
this.pendingTestsLogReference.get(event.testFile)?.set(event.title, event.logs);

@novemberborn novemberborn changed the title Output logs for pending tests after global timeout or SIGINT is triggered Output logs for tests that remain pending when AVA exits Nov 13, 2022
Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely!

@novemberborn novemberborn merged commit 9206928 into avajs:main Nov 13, 2022
13 checks passed
@kevo1ution kevo1ution deleted the fix-issue-3222-log-after-timeout branch November 13, 2022 22:02
@kevo1ution
Copy link
Contributor Author

kevo1ution commented Nov 13, 2022

Lovely!

Thank you for the helpful reviews and insight on fixing the issue! Looking forward to using this fix in my repos

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.

t.log() does not output anything if an async test fails due to time out
2 participants