Add tests for routing util #582
Conversation
|
socsieng
force-pushed
the
tests/routing-util
branch
from
7ecf9fd to
0dda8bd
Compare
socsieng
force-pushed
the
tests/routing-util
branch
from
0dda8bd to
e288544
Compare
| it("returns url parts for /", () => { | ||
| expect(getUrlParts("/", false)).toEqual({ | ||
| hostname: "", | ||
| pathname: "", // TODO: This behaviour is inconsistent with external pathname |
There was a problem hiding this comment.
What do you mean it's inconsistent ?
There was a problem hiding this comment.
External pathnames start with / where internal ones don't. Examples:
- Internal:
- url:
"/"-> pathname:"" - url:
"/path"-> pathname:"path"
- url:
- External:
- url:
"http://localhost/"-> pathname:"/" - url:
"http://localhost/path"-> pathname:"/path"
- url:
There was a problem hiding this comment.
Ho right, we'll need to look into that
| }); | ||
|
|
||
| it("converts mixed multi-value and single value parameters", () => { | ||
| const querystring = "key=value1&key=value2&another=value3"; |
There was a problem hiding this comment.
It would be great to test if the multi-value key are not following each other
| describe("regex", () => { | ||
| describe("escapeRegex", () => { | ||
| it("should escape (.) with _µ1_", () => { | ||
| const result = escapeRegex("/a(.)b"); |
There was a problem hiding this comment.
For this and all the other below, it would be great to use some realistic example. This is used for intercepting route
There was a problem hiding this comment.
I'm not sure how this is actually used. Are you able to provide some examples that I can incorporate?
There was a problem hiding this comment.
It's for interception route https://nextjs.org/docs/app/building-your-application/routing/intercepting-routes
it would be /a/(.)b for example
| }); | ||
| }); | ||
|
|
||
| describe.skip("TODO: proxyRequest", () => {}); |
There was a problem hiding this comment.
This one will be hard to properly test with unit test. This is one that we want to test mostly on e2e
There was a problem hiding this comment.
Yeah, I thought it would be difficult too.
Did you want me to remove this altogether or update the description to say that it covered by e2e tests?
There was a problem hiding this comment.
Updating the description makes more sense so that if someone change they'll know that they should implement test in e2e
| }; | ||
|
|
||
| addOpenNextHeader(headers); | ||
| expect(headers).toHaveProperty("X-OpenNext-Version"); |
There was a problem hiding this comment.
Why not test the value as well ?
2 participants
I split the change into two commits. One with the additional tests and another to rename the file.