-
Notifications
You must be signed in to change notification settings - Fork 5
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
32129139: Improve some unit tests #487
Conversation
1335074
to
1239e04
Compare
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.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @BenjaminVanRyseghem, @DamienCassou, @danglotb, @JohanBriglia, and @josquindebaz)
src/test/router/routerTest.js
line 187 at r4 (raw file):
// Assert that callback was executed expect(spy).toHaveBeenCalledWith(jasmine.anything()); });
Why is this assertion better than the previous one?
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.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @BenjaminVanRyseghem, @danglotb, @JohanBriglia, @josquindebaz, and @ValentinaVasile)
src/test/router/routerTest.js
line 187 at r4 (raw file):
Previously, ValentinaVasile (Valentina Vasile) wrote…
Why is this assertion better than the previous one?
it checks the function was called with one and only one argument. Not much better I agree but some eslint rules forbid the use of toHaveBeenCalled()
.
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.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @BenjaminVanRyseghem, @DamienCassou, @danglotb, @JohanBriglia, and @josquindebaz)
src/test/router/routerTest.js
line 187 at r4 (raw file):
Previously, DamienCassou (Damien Cassou) wrote…
it checks the function was called with one and only one argument. Not much better I agree but some eslint rules forbid the use of
toHaveBeenCalled()
.
thanks for the answer
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.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @BenjaminVanRyseghem, @DamienCassou, @danglotb, and @JohanBriglia)
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @BenjaminVanRyseghem, @DamienCassou, @danglotb, and @JohanBriglia)
1239e04
to
3e91982
Compare
https://3.basecamp.com/4201305/buckets/32129139/todos/7278579439
This change is