Add tests for atoms toast templates#77
Conversation
SaidShah
requested review from
brandongregoryscott,
jstevenson and
wintondeshong
Codecov Report
@@ Coverage Diff @@
## main #77 +/- ##
==========================================
+ Coverage 25.66% 34.25% +8.59%
==========================================
Files 51 51
Lines 900 902 +2
Branches 209 209
==========================================
+ Hits 231 309 +78
+ Misses 662 591 -71
+ Partials 7 2 -5
Continue to review full report at Codecov.
|
brandongregoryscott
left a comment
brandongregoryscott
left a comment
There was a problem hiding this comment.
Thanks for filling these in @SaidShah! Left some feedback for breaking up these test suites a bit - I think it should help with future addition or maintenance to the tests as well as readability
| test.skip("TODO: Backfill tests for issue https://github.com/AndcultureCode/AndcultureCode.JavaScript.React.Components/issues/34", () => {}); | ||
| test("when error toast created, returns toastId", () => { | ||
| // Arrange | ||
| const testToastId = faker.random.number(99); |
There was a problem hiding this comment.
Thought: Being as this is the value we're expecting to be returned from the function under test, this would be a good candidate to be named expected 👍
|
|
||
| describe("ToastManager", () => { | ||
| test.skip("TODO: Backfill tests for issue https://github.com/AndcultureCode/AndcultureCode.JavaScript.React.Components/issues/34", () => {}); | ||
| test("when error toast created, returns toastId", () => { |
There was a problem hiding this comment.
We typically wrap each function being tested in its own ts-region block and a describe block. While these sections may only contain one test right now, it's more consistent with the rest of our test suites and will be easier to maintain if additional tests need to be added in here.
In terms of naming sequential tests, it could also reduce some of the boilerplate due to the context:
// -----------------------------------------------------------------------------------------
// #region info
// -----------------------------------------------------------------------------------------
describe("info", () => {
test("when toastId provided, returns toastId", () => {
// Arrange
const expected = faker.random.number(99);
const toastContent = faker.random.word();
const toastOptions = { toastId: expected };
// Act
const toastId = ToastManager.info(toastContent, toastOptions);
// Assert
expect(toastId).toBe(expected);
});
})
// #endregion info
I'd recommend a similar change for the toast template tests ☝️
| // Arrange | ||
| const testToastId = faker.random.number(99); | ||
| const toastContent = faker.random.word(); | ||
| const toastOptions = { toastId: testToastId }; |
There was a problem hiding this comment.
It should return an id of the created toast without explicitly passing one in, right? Can we add test(s) for those cases?
| expect(dismissMethodSpy).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test("when dismiss All toasts, calls dismissAll method", () => { |
There was a problem hiding this comment.
The suggested region/describe blocks would probably help with this test name
There was a problem hiding this comment.
@brandongregoryscott - Thanks for your feedback. Can you take a second look when you have time.
brandongregoryscott
left a comment
brandongregoryscott
left a comment
There was a problem hiding this comment.
@SaidShah Just a tiny update here and it's all set 👍 Thanks for making those changes!
|
|
||
| // Assert | ||
| expect(dismissAllMethodSpy).toHaveBeenCalled(); | ||
| describe("dismissAll", () => { |
There was a problem hiding this comment.
Just one tiny nitpick @SaidShah: could you alphabetize these regions? If the functions in the class are not alphabetized, they should be moved around too.
There was a problem hiding this comment.
Thanks for the feedback. I have updated the regions if you want to take another look. 👍
3 participants
Closes #34