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

test: Use a different way to update window object in tests #2095

Merged
merged 2 commits into from
Nov 24, 2023

Conversation

gkarat
Copy link
Contributor

@gkarat gkarat commented Nov 15, 2023

This is an attempt to fix the coverage random error happening on Travis CI while executing component tests.

@gkarat gkarat self-assigned this Nov 15, 2023
@gkarat gkarat requested a review from a team as a code owner November 15, 2023 10:53
},
},
getApp: () => 'inventory',
window.chrome = {
Copy link
Member

Choose a reason for hiding this comment

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

Would mocking all this with the useChrome override not make the use of the window object obsolete?

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 is true, it seems this property modification is already obsolete. I will try now to remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in the next commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I didn't remove the window.insights mock because some parts of the code (but not Inventory's) still refers to window.insights.chrome object for some reason.

(uncaught exception)Error: Objects are not valid as a React child (found: TypeError: Cannot read properties of undefined (reading 'chrome')). If you meant to render a collection of children, use an array instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be remnants of the chrome older version which embedded the chrome into the window heavily. Maybe if we find what packages are still using that old version and update them, we can remove this final bit as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we can. But not sure it will contribute much to the issue with the tests I am trying to solve currently. I will keep you note in mind and fix in a separate PR.

Also merging this. Looks like at least 2 people took a look and there is almost no way to verify this PR, so let's see if it heals CI on master.

@gkarat gkarat requested review from a team and bastilian November 15, 2023 11:31
@gkarat
Copy link
Contributor Author

gkarat commented Nov 15, 2023

/retest

@gkarat gkarat changed the title test: Use different way to update window object test: Use a different way to update window object Nov 15, 2023
@gkarat gkarat changed the title test: Use a different way to update window object test: Use a different way to update window object in tests Nov 15, 2023
@gkarat gkarat force-pushed the fix-cypress-coverage branch from 07609f8 to 7a3136c Compare November 15, 2023 14:06
@gkarat gkarat added the tests label Nov 21, 2023
@gkarat gkarat merged commit aee1350 into RedHatInsights:master Nov 24, 2023
2 checks passed
@gkarat gkarat deleted the fix-cypress-coverage branch November 24, 2023 10:05
@gkarat
Copy link
Contributor Author

gkarat commented Nov 24, 2023

🎉 This PR is included in version 1.59.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

3 participants