-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Next.js integration tests for Server and Browser #3632
Conversation
size-limit report
|
b694ec3
to
7ac9d4c
Compare
packages/nextjs/test/integration/test/browser/sessionNavigate.js
Outdated
Show resolved
Hide resolved
let scenarios = await fs.readdir(path.resolve(__dirname, './server')); | ||
|
||
if (FILES_FILTER) { | ||
scenarios = scenarios.filter(file => file.toLowerCase().includes(FILES_FILTER)); |
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.
Why filtering without considering the case? Also, any condition that's not the exact same (partial) match should be documented on line 19.
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.
Purely for convinience. It just annoyed me that I typed node server.js api
and it didn't find the test ¯_(ツ)_/¯
I dont understand the second part of your feedback. Line 19 states that it'll match on partials 🤔
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.
What I understand from "partial match" is it can be a substring, not that we're not considering the case. I'm good for not considering it, but I feel we should not that.
if ( | ||
isSentryRequest(request) || | ||
// Used for testing http tracing | ||
request.url().includes('example.com') |
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.
example.com
(http://example.com
in other places) is required, or can any URL be used? If it has to be this one, is there any other way to use (or modify) it, instead of hardcoding it everywhere?
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.
Added missing protocol. Any URL can be used, but it has to match, that's why I used the same one everywhere.
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.
LGTM.
return true; | ||
}; | ||
|
||
// Misc |
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'd separate this misc stuff from the server stuff into two separate files, but this is a minor thing.
Completely custom test runner and integration tests for Next.js application. Covers both, server and browser environments.
Currently implements the most common use cases of error handling, tracing, and sessions, but the suites are definitely non-exhaustive.
Application has to be pre-built using
yarn build
(covered by thenpm run test:integration
) and tests can be run using node directly or through npm scripts. The runner allows for filtering specific test cases and provides debug mode for logging intercepted requests and enabling console logs.Only the second commit is reviewable. The first one is completely isolated example app and it doesn't require a review, although feel free to do so if you feel like it.