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

fix(nextjs): Small integration test runner fixes #3801

Merged
merged 10 commits into from
Jul 14, 2021

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Jul 13, 2021

This PR fixes a few small bugs in our test runner for nextjs integration tests:

  • Debug mode failed because of a missing import
  • Test runner options couldn't be passed except by hard-coding them in the bash script called by yarn
  • One of our node/nextjs compatibility checks was checking the wrong version of nextjs
  • Console breadcrumbs caused ever-larger events to be logged, since each time an object including breadcrumbs was logged, the entire object then became a breadcrumb in the next object logged, which itself became a breadcrumb in the next one, and so on and so forth
  • In the test-runner bash script, our variable keeping track of exit code was reset unnecessarily (if it's anything other than 0 by that point, the script already will have bailed)
  • Our test filtering was case-sensitive

It also makes a few small changes to make debugging those tests easier:

  • When comparing objects, the actual value is now stored as a variable, so that the value will be shown inline by the debugger
  • The default value of the depth option is now included in its help text
  • console.log(inspect(...)) was changed to console.dir(...), which does both in one, and color is now included in the output

@github-actions
Copy link
Contributor

github-actions bot commented Jul 13, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 21.46 KB (0%)
@sentry/browser - Webpack 22.47 KB (0%)
@sentry/react - Webpack 22.5 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 28.95 KB (-0.01% 🔽)

@@ -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');
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 { inspect } = require('util');

Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Member

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.

@kamilogorek
Copy link
Contributor

Could you also please add these 3 changes?

image

They were overwritten in the #3788 PR, but thankfully I have local copy 😅

@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-test-runner-fixes-july-2021 branch from 6605420 to 039dfa6 Compare July 14, 2021 13:27
@@ -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 });
Copy link
Contributor

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

@lobsterkatie
Copy link
Member Author

Could you also please add these 3 changes?
They were overwritten in the #3788 PR, but thankfully I have local copy 😅

Shoot, sorry about that. I've added them in.

@lobsterkatie lobsterkatie merged commit 85f2fe8 into master Jul 14, 2021
@lobsterkatie lobsterkatie deleted the kmclb-nextjs-test-runner-fixes-july-2021 branch July 14, 2021 15:43
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.

None yet

3 participants