-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(nextjs): Small integration test runner fixes #3801
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
fix(nextjs): Small integration test runner fixes #3801
Conversation
size-limit report
|
@@ -2,6 +2,7 @@ const { createServer } = require('http'); | |||
const { parse } = require('url'); | |||
const { stat } = require('fs').promises; | |||
const next = require('next'); | |||
const { inspect } = require('util'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const { inspect } = require('util'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid only after utils/common.js
replaces it with JSON.stringify
. But then depth
option wont work unless substituted with a correct depth-aware stringifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hang on, though - what's the reason we want to replace inspect
with JSON.stringify
? What if we just use console.dir()
, as it takes a depth argument and also can print different value types in different colors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot about the depth option, we should just keep using inspect
then, I don't think we want to replace.
Could you also please add these 3 changes? They were overwritten in the #3788 PR, but thankfully I have local copy 😅 |
6605420
to
039dfa6
Compare
@@ -37,7 +37,7 @@ const logIf = (condition, message, input, depth = 4) => { | |||
if (condition) { | |||
console.log(message); | |||
if (input) { | |||
console.log(inspect(input, { depth })); | |||
console.dir(input, { depth, colors: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL console.dir
accept options in node api
Shoot, sorry about that. I've added them in. |
This PR fixes a few small bugs in our test runner for nextjs integration tests:
0
by that point, the script already will have bailed)It also makes a few small changes to make debugging those tests easier:
depth
option is now included in its help textconsole.log(inspect(...))
was changed toconsole.dir(...)
, which does both in one, and color is now included in the output