test: fix test code improperly using/missing 'await'#2560
test: fix test code improperly using/missing 'await'#2560bradenmacdonald merged 13 commits intoopenedx:masterfrom
Conversation
|
Thanks for the pull request, @bradenmacdonald! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
64aa28c to
3495f28
Compare
3495f28 to
d6e348e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2560 +/- ##
==========================================
+ Coverage 94.74% 94.77% +0.02%
==========================================
Files 1218 1218
Lines 27312 27330 +18
Branches 5989 6147 +158
==========================================
+ Hits 25878 25901 +23
+ Misses 1376 1358 -18
- Partials 58 71 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@ChrisChV I found (and fixed) a lot of broken tests in this MFE. Can you please review? |
ChrisChV
left a comment
There was a problem hiding this comment.
@bradenmacdonald Thanks for all this work! Looks good! I added some non-bloker nits 👍
- I tested this: I verified that the tests are green
- I read through the code and considered the security, stability and performance implications of the changes.
- Includes tests for bugfixes and/or features added.
| fireEvent.click(addButton); | ||
| }); | ||
| it('should create another instructor form on click Add new instructor', async () => { | ||
| const { getAllByRole, getByRole, rerender } = render(<RootWrapper {...props} />); |
There was a problem hiding this comment.
Nit: use screen here for getAllByRole, getByRole, rerender
There was a problem hiding this comment.
Sure. Though I don't think rerender is available any other way except how it's returned from render.
Description
I'm testing out
oxlinton this repo to see what issues it finds, and it's finding lots :/This PR fixes missing
awaitstatements in test code (which is a serious issue as it means some test code was being completely ignored), as well as incorrectawaitstatements that were awaiting non-promises (which is not a big deal, but still bad practice).This does not fix all the issues with missing
await. There are a lot more, but I'm leaving ones that affect the non-test code for a separate PR, and ignoring some like calls toinvalidateQueries, where I don't think it really matters if we await them or not.Supporting information
Part of #2559
Private ref MNG-4763
Testing instructions
Just ensure tests are passing - there is no change to the MFE code outside of
.test.xfiles.Best Practices Checklist
We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:
.ts,.tsx).propTypes,defaultProps, andinjectIntlpatterns are not used in any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'