Improve tests #65
Improve tests #65
Conversation
test/route.test.js
Outdated
| const state = { location: { pathname: `/foo/${invalidChars}/bar/baz` } } | ||
| const actions = {} | ||
| const render = jest.fn(({ match }) => { | ||
| expect(match.params).toEqual({ bar: "baz" }) |
There was a problem hiding this comment.
@jorgebucaran Is this intended behavior? In my opinion, a malformed URI should not match to any routes. What do you think?
There was a problem hiding this comment.
Why not toEqual({ foo: "%E0%A4%A", bar: "baz" })
There was a problem hiding this comment.
Removing decodeURI from ↓ changes match.params from { bar: "baz" } to { foo: "%E0%A4%A", bar: "baz" }. I'm not sure why decodeURI is used here.
There was a problem hiding this comment.
Decoding is make sense because you usually expect { name: "café" } instead of { name: "caf%C3%A9" } in the params object.
but probably the code above should looks like this:
function decodeParam(val) {
try {
return decodeURIComponent(val)
} catch (err) {
return val
}
}
params[paths[i].slice(1)] = urls[i] = decodeParam(urls[i])
There was a problem hiding this comment.
@yuku-t In my opinion, a malformed URI should not match to any routes. What do you think?
A lot of code was golfed to make the router as small as possible. It was fun, but probably a waste of time. I am not too obsessed about trading bytes for preciseness anymore and I think it's time to revisit a lot of the implementation code.
Codecov Report
@@ Coverage Diff @@
## master #65 +/- ##
========================================
+ Coverage 96.92% 100% +3.07%
========================================
Files 6 6
Lines 65 68 +3
Branches 10 11 +1
========================================
+ Hits 63 68 +5
+ Misses 2 0 -2
Continue to review full report at Codecov.
|
| [ | ||
| "transform-react-jsx", | ||
| { | ||
| "pragma": "h" |
test/route.test.js
Outdated
| const state = { location: { pathname: `/foo/${invalidChars}/bar/baz` } } | ||
| const actions = {} | ||
| const render = jest.fn(({ match }) => { | ||
| expect(match.params).toEqual({ bar: "baz" }) |
There was a problem hiding this comment.
@yuku-t In my opinion, a malformed URI should not match to any routes. What do you think?
A lot of code was golfed to make the router as small as possible. It was fun, but probably a waste of time. I am not too obsessed about trading bytes for preciseness anymore and I think it's time to revisit a lot of the implementation code.
| }, | ||
| "jest": { | ||
| "testURL": "http://localhost" | ||
| }, | ||
| "devDependencies": { | ||
| "babel-jest": "^22.4.3", | ||
| "babel-plugin-transform-react-jsx": "^6.24.1", |
There was a problem hiding this comment.
@yuku-t I don't expect you to write all tests to use JSX, but could you use JSX in these tests? 🙏
| const vnode = h(Link, { to: "/path", onclick })({}, {}) | ||
| test("Calling onclick of VNode transparently calls Link's onclick prop", () => { | ||
| const onclickProp = jest.fn() | ||
| const vnode = Link({ onclick: onclickProp })({}, {}) |
There was a problem hiding this comment.
It is intentional design. There are two reasons:
-
Because of lazy component, the mentioned line equals to the following. Don't you feel strange?
const vnode = (<Link onclick={onclickProp} />)({}, {})
-
test/link.test.js is for unit testing.
Linkcould be used as a component, but it is just a pure function.- On the other hand, test/index.test.js is for integration testing, so I used JSX there.
There was a problem hiding this comment.
Okay, makes sense. That would be weird indeed.
test/route.test.js
Outdated
| expect(match.params).toEqual({ bar: "baz" }) | ||
| }) | ||
| expect(() => | ||
| Route({ path: "/foo/:foo/bar/:bar", render }, [])(state, actions) |
|
Pushed some commits:
|
| const wait = async ms => new Promise(resolve => setTimeout(resolve, ms)) | ||
| const click = e => e.dispatchEvent(new MouseEvent("click", { button: 0 })) | ||
|
|
||
| let state, actions, unsubscribe |
There was a problem hiding this comment.
I try to avoid using var and let when writing pure ES6, but I can fix this later!
MouseEventinstead of callingvnode.attributes.onclick.app()in their test code) to test/index.test.js.Route,Linkandlocation.Linkonly. It concentrates on unit testing.