test(dependency): Improve test coverage #2759
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
| Playground | Link |
|---|---|
| React demo | https://livecodes.io?x=id/27KQLS2MD |
See documentations for usage instructions.
tests/vanilla/dependency.test.tsx
Outdated
| it('handles errors in async atoms', async () => { | ||
| const errorAtom = atom(async () => { | ||
| throw new Error('Test error') | ||
| }) | ||
|
|
||
| const store = createStore() | ||
|
|
||
| await expect(store.get(errorAtom)).rejects.toThrow('Test error') | ||
| }) |
There was a problem hiding this comment.
This doesn't seem a dependency test. Maybe move to some other file?
There was a problem hiding this comment.
@dai-shi Appreciated for comments.
You're absolutely right, it should be moved out of here. These changes have been removed from this pull request at this moment, after that, this pull request will be goal-focused enough and for dependency test only. Descriptions for this pull request has been updated as well already.
tests/vanilla/dependency.test.tsx
Outdated
| const derived2 = atom((get) => get(derived1) + 1) | ||
| const asyncDerived = atom(async (get) => { | ||
| const value = get(derived2) | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) |
There was a problem hiding this comment.
We don't use timers in the test.
Please do something like this:
jotai/tests/vanilla/dependency.test.tsx
Line 11 in 9bd8ed7
tests/vanilla/dependency.test.tsx
Outdated
| const atoms = new Array(1000).fill(null).map((_, i) => atom(i)) | ||
| const derivedAtom = atom((get) => atoms.reduce((sum, a) => sum + get(a), 0)) | ||
|
|
||
| const start = performance.now() |
There was a problem hiding this comment.
Hmm, I don't think we do performance tests in our unit tests.
Can you try something with ./benchmarks? It's not a test that runs on CI, but you can try more complex cases and share the result in a discussion.
There was a problem hiding this comment.
I think the existing benchmark suite also needs some refinement. I did some resource tracking a while back and memory consumption was not lining up with what I thought the tests are testing for
There was a problem hiding this comment.
I think the existing benchmark suite also needs some refinement.
I actually agree. Not just refinement, you can create a brand new benchmark suite. We can even delete existing one, if yours covers it. That would be pretty helpful.
There was a problem hiding this comment.
@backbone87 Appreciated for sharing detections you noticed before. @dai-shi I prefer to leave this topic aside and it should be independently handled by another clear and goal-focused pull request. These changes have been removed at this moment. 🙇
Summary
This pull request enhances the test suite for Jotai's dependency management, particularly focusing on complex scenarios. The following improvement has been made:
New Test Cases
Coverage Improvements
Check List
Appreciated for having me your time on reviewing changes.