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

chore(deps): update paragon and frontend-build to openedx scope #818

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

brian-smith-tcril
Copy link
Contributor

No description provided.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
"@edx/reactifex": "^1.0.3",
"@edx/stylelint-config-edx": "^2.3.0",
"@edx/stylelint-config-edx": "2.3.0",
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 was on 2.3.0 due to the package-lock.json, after rebuilding the package lock to cleanly handle the scope changes it updated to 2.3.3 and caused https://github.com/openedx/frontend-app-course-authoring/actions/runs/7746261151/job/21124075247 to fail with errors such as

src/index.scss
  1:9  ✖  Expected ""~@edx/brand/paragon/fonts"" to be "url("~@edx/brand/paragon/fonts")"                                                import-notation

Pinning it to 2.3.0 prevents those errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

No objections. We can deal with that later.

Comment on lines +52 to +65
class ResizeObserver {
observe() {
// do nothing
}

unobserve() {
// do nothing
}

disconnect() {
// do nothing
}
}

window.ResizeObserver = ResizeObserver;
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 was getting "ResizeObserver is not defined" errors. I decided to see how paragon is handling those and I found https://github.com/openedx/paragon/blob/7e4a81f9f2350c54362baa852c75dc32c2c2694e/src/setupTest.js#L7-L21, so I copied that here.

const { container } = renderComponent();
expect(container).toBeEmptyDOMElement();
const { queryByTestId } = renderComponent();
expect(queryByTestId('browser-router')).toBeEmptyDOMElement();
Copy link
Contributor Author

@brian-smith-tcril brian-smith-tcril Feb 16, 2024

Choose a reason for hiding this comment

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

As of openedx/frontend-platform#615, the AppProvider used in renderComponent includes

<div data-testid="browser-router">

and the OptionalReduxProvider in the AppProvider includes

<div data-testid="redux-provider">

so instead of the whole container being an empty DOM element, it is

<div data-testid="redux-provider"><div data-testid="browser-router"></div></div>

This just updates the test to ensure nothing is rendered inside those divs that are always rendered

@brian-smith-tcril brian-smith-tcril marked this pull request as ready for review February 16, 2024 00:16
Copy link

codecov bot commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (51c5f9c) 89.27% compared to head (aeb0ee5) 89.27%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #818   +/-   ##
=======================================
  Coverage   89.27%   89.27%           
=======================================
  Files         551      551           
  Lines        9738     9738           
  Branches     2099     2099           
=======================================
  Hits         8694     8694           
  Misses        996      996           
  Partials       48       48           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

Congrats on making all tests pasts, and thanks for getting to the bottom of it!

@arbrandes arbrandes merged commit 76bb8e8 into openedx:master Feb 16, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants