[BD-29] [TNL-7268] Add high priority tests - #97
Conversation
|
Thanks for the pull request, @Agrendalath! I've created BLENDED-394 to keep track of it in Jira. More details are on the BD-29 project page. When this pull request is ready, tag your edX technical lead. |
openedx-webhooks
added
needs triage
open-source-contribution
There was a problem hiding this comment.
@Agrendalath
I've never come across any of this, so I definitely feel a little out of place with reviews, but ill go back and take a look through some jest documentation before I come back for a full review round. So far this is looking good though. I did notice you have some snapshots that seem unused, and npm test fails, listing those obsolete snapshots as the reason.
› 5 snapshots obsolete.
• Unit Navigation displays loading message 1
• Unit Navigation displays message for no content 1
• Unit Navigation displays message for the locked content 1
• Unit Navigation displays messages for the locked content 1
• Unit Navigation displays messages for the locked content 2
Also, package-lock.json is conflicting
a3c2075 to
01bf231
Compare
Agrendalath
changed the title
There was a problem hiding this comment.
Several questions and a few nitpicks, but overall, it looks good. Would like to hear from you regarding the comments though.
Leaving this on Approved (meant to do it with this, review, but messed that up, so it was done separately below) so you can respond and address concerns and not be blocked on me though since none of the comments are blocking.
- I tested this: ran
npm i && npm testand received passing tests
Test Suites: 9 passed, 9 total
Tests: 73 passed, 73 total
Snapshots: 34 passed, 34 total
- I read through the code
|
@Agrendalath Thank you for your contribution. Please let me know once this is ready for review. |
openedx-webhooks
added
waiting on author
|
It's on me for not getting this note into the TNL-7268 ticket, but I'm wondering if we can focus on specific assertions rather than snapshot tests - the latter are very brittle and difficult to maintain. We should be mindful not to over-assert on implementation/styling details in component tests, which is basically what a snapshot does. Doing so makes it much more likely to break on inconsequential changes. (Similar comment over here: openedx/frontend-app-authoring#2 (comment)) Making specific assertions also helps with maintainability by signaling intent. It helps us understand what specific things we consider our acceptance criteria - i.e., the sequence nav appears with the right number of buttons. Put another way, these tests shouldn't fail if we add another Let me know what you think - thanks! |
openedx-webhooks
added
changes requested
and removed
waiting on author
marcotuts
changed the title
|
Note, I just merged https://github.com/edx/frontend-app-learning/pull/102 which does some code organization. I don't expect it should affect these tests too much beyond needing to adjust some imports, perhaps. The contents of the files didn't change for the most part, I just moved some things around. Let me know if you have any issues. |
d9d44e1 to
8db73e9
Compare
|
FYI, I've added some tests for the CoursewareContainer here: https://github.com/edx/frontend-app-learning/pull/108 As part of that, I used the rosie factories from the data integration tests to set up a redux store, and modified them so that they could allow for multiple units (useful for testing sequence navigation, presumably). Not all components will need the full store, but it was pretty nice being able to set up the store and simply mock the API requests. I noticed in this PR here there's a bunch of manual set up of redux store states... it might be more resilient to take the approach pioneered in those data int tests. Manually setup stores have a tendency to diverge from what gets set up by actions. (i.e., if someone changes the store, they don't know to go update the manual testing state) |
d8bece2 to
09b322a
Compare
Codecov Report
@@ Coverage Diff @@
## master #97 +/- ##
==========================================
+ Coverage 53.43% 62.21% +8.77%
==========================================
Files 113 113
Lines 1295 1310 +15
Branches 258 267 +9
==========================================
+ Hits 692 815 +123
+ Misses 581 476 -105
+ Partials 22 19 -3
Continue to review full report at Codecov.
|
|
@davidjoy, as mentioned on Slack, it totally makes sense to use factories here to have more reliable and consistent approach. I rebased the branch and made some amends (e.g. global.MutationObserver issue for async functions has been fixed after updating dependencies (yay!) and the FontAwesome icon mock turned out to be redundant, as we're not using the snapshot tests eventually). |
|
@davidjoy, a quick update: it seems to be a bit more tricky than I expected. For now I'm using something similar to this snippet for providing a generic way to create stores for tests. I'm invoking it this way: beforeEach(async () => {
store = await initializeTestStore();
const sequence = Object.values(store.getState().models.sequences)[0];
unit = store.getState().models.units[sequence.unitIds[0]];
mockData = {
unitId: unit.id,
onClick: () => {},
};
});The solution from #108 is very verbose, but if we end up using it for all existing tests, we'll end up with tons of boilerplate code that will make them very hard to read. Currently I'm looking for a way to customize the generated metadata without duplicating too many lines fo code. |
|
I think the above We should be able to update the store to put it in specific states by dispatching actions/calling thunks. I added that "buildSimpleCourseBlocks" function to try to reduce some of the boilerplate I was seeing... I think it'd be reasonable for the factories to have helpers like that that abstract away a little of the complexity of modifying the factory build. Because all the factories need to interact in very specific ways, it'd be helpful for future test writers to have some helpers they can use easily. Long story short, I think adding this setup makes sense. |
As we're not using snapshots, we will not need this anymore.
5534308 to
7e07bb0
Compare
|
@davidjoy, I pushed the refactored version that uses factories and There are some minor changes to be done (e.g. renaming |
| const courseMetadata = Factory.build('courseMetadata'); | ||
| const courseId = courseMetadata.id; | ||
| const { courseBlocks, unitBlock, sequenceBlock } = buildSimpleCourseBlocks(courseId); | ||
| const { courseBlocks, unitBlocks, sequenceBlock } = buildSimpleCourseBlocks(courseId); |
There was a problem hiding this comment.
Seems like sequenceBlock is expected to come back as an array? But I don't see that the thing being returned from buildSimpleCourseBlocks has an array for sequenceBlock - it's just the block. A bit confused how it's working.
There was a problem hiding this comment.
It's an array, if the options.sequenceBlock is undefined. I'll reword this to plural to make it less confusing.
Edit: linking the related snippet here, because GitHub doesn't seem to be scrolling to the code from the link for some reason:
const sequenceBlock = options.sequenceBlock || [Factory.build(
'block',
{ type: 'sequential', children: unitBlocks.map(block => block.id) },
{ courseId },
)];
openedx-webhooks
added
blended
|
FYI - I've rebased and merged this into If this is successful, I should be able to merge that in shortly. |
|
Don't worry about resolving these conflicts for the moment - going to give it a shot myself. |
|
Cool, thank you for tackling this, @davidjoy! |
This is effectively fixing merge conflicts between: https://github.com/edx/frontend-app-learning/pull/128 and: https://github.com/edx/frontend-app-learning/pull/97
This is effectively fixing merge conflicts between: https://github.com/edx/frontend-app-learning/pull/128 and: https://github.com/edx/frontend-app-learning/pull/97
This is effectively fixing merge conflicts between: https://github.com/edx/frontend-app-learning/pull/128 and: https://github.com/edx/frontend-app-learning/pull/97
|
Heyo - so I resolved the merge conflicts here: https://github.com/edx/frontend-app-learning/pull/136 I merged that in, so we're good here! Thanks @Agrendalath! I'm going to close this one. We'll have to make sure the JIRA tickets and all that get resolved properly. |
5 participants
This adds tests for:
Testing instructions:
npm i && npm test.Reviewers