Skip to content
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

fix: act warnings fix, and also fix proptypes error in CatalogNoResultsDeck #199

Merged
merged 1 commit into from Feb 28, 2022

Conversation

binodpant
Copy link
Contributor

@binodpant binodpant commented Feb 28, 2022

With node16 (coming up) many of these will become errors, due to this: https://developer.ibm.com/blogs/nodejs-15-release-blog

This PR unblocks the node16 PR: #198

This PR fixes:

  • CatalogNoResultsDeck has a proptypes error since undefined data was being set. Instead I changed it to stay [] in this case. This fixes prop types warning (which is actually an error in node16)
  • fixes act warnings in the test overall by awaiting various elements in ui before returning. This also unblocks node16 (for this test at least)

For all changes

  • Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

  • Ensure to attach screenshots
  • Ensure to have UX team confirm screenshots

@@ -38,7 +38,7 @@ const CatalogNoResultsDeck = ({
useEffect(() => {
const defaultCoursesRefinements = { enterprise_catalog_query_titles: selectedCatalog, content_type: contentType };
EnterpriseCatalogApiService.fetchDefaultCoursesInCatalogWithFacets(defaultCoursesRefinements).then(response => {
setDefaultData(response.default_content);
setDefaultData(response.default_content || []);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured keeping this as the default value of [] was fine, since in a test, we are somehow passing undefined to setDefaultData()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fixes the proptypes warning

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because DataTable has set data to be required

@@ -188,6 +188,7 @@ describe('Main Catalogs view works as expected', () => {
expect(screen.queryByText(TEST_PARTNER_2)).toBeInTheDocument();

expect(screen.queryAllByText('A la carte').length === 2);
await act(() => screen.findByText('Business'));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general, the theme here is: wait for an async activity in UI to conclude before returning test, and wrap that wait in an act(). This simulates what a user experiences and also avoid failures in tests due to side effects that may run after tests return

@codecov
Copy link

codecov bot commented Feb 28, 2022

Codecov Report

Merging #199 (018c2f8) into main (1556a0c) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #199   +/-   ##
=======================================
  Coverage   81.98%   81.98%           
=======================================
  Files          37       37           
  Lines         544      544           
  Branches      156      157    +1     
=======================================
  Hits          446      446           
  Misses         94       94           
  Partials        4        4           
Impacted Files Coverage Δ
...ents/catalogNoResultsDeck/CatalogNoResultsDeck.jsx 87.50% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1556a0c...018c2f8. Read the comment docs.

Copy link
Contributor

@alex-sheehan-edx alex-sheehan-edx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! ✅

@binodpant binodpant merged commit cf86dd5 into main Feb 28, 2022
@binodpant binodpant deleted the bpant/act-warnings-unblock-node16 branch February 28, 2022 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants