test: added tests for notifications reducer#567
Conversation
alejandronanez
left a comment
alejandronanez
left a comment
There was a problem hiding this comment.
Hey @jglover thanks for this PR, I think it's pretty clear and well written.
I have 2 things I'd like to see changed here:
- Try not to use relative imports
- Avoid nested describes for your tests
Besides that, LGTM.
|
|
||
| import organization from '../../data/organization'; | ||
| import user from '../../data/user'; | ||
| import organization from '../../data/api/organization'; |
There was a problem hiding this comment.
Do you think we can use absolute imports instead of relative imports? Relative imports can get messy pretty quick.
There was a problem hiding this comment.
This occurred to me, I'll update it.
There was a problem hiding this comment.
Hmmm we're still using relative imports here.
| @@ -0,0 +1,475 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Generally speaking, I agree with @andrewda about not nesting describes, that can get quite messy pretty quickly.
I think one describe per test suite is fine and just using it clauses should be enough for describing a good test suite. Can you please flatten your tests so you only use 1 describe and multiple it statements?
| import { notification } from '../../data/api/notification'; | ||
| import { authError } from '../../data/api/error'; | ||
|
|
||
| describe('Notifications reducer', () => { |
There was a problem hiding this comment.
Use title case: "Notifications Reducer".
|
@alejandronanez @andrewda The requested changes have been made :) |
|
|
||
| import organization from '../../data/organization'; | ||
| import user from '../../data/user'; | ||
| import organization from '../../data/api/organization'; |
There was a problem hiding this comment.
Hmmm we're still using relative imports here.
| open as openPr, | ||
| merged as mergedPr, | ||
| } from '../../data/pull-request'; | ||
| } from '../../data/api/pull-request'; |
There was a problem hiding this comment.
Also we're using relative imports here.
There was a problem hiding this comment.
These are just tests I touched when I moved the test data files, my discipline tells me update all tests in a different PR but I'll do it here.
There was a problem hiding this comment.
@alejandronanez all references updated to use alias instead of absolute paths.
…git-point into add-notifications-reducer-tests
|
@alejandronanez updated paths to use alias instead of absolute paths across all tests. |
|
Thanks! |
5 participants
Added tests for notifications reducer. This revealed a few problems with the reducer that I'll address in an issue.