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

refactor(tests/e2e): simplify openPopup helper #706

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

sidvishnoi
Copy link
Member

Discovered when writing article for end-to-end testing 😄

@github-actions github-actions bot added the area: tests Improvements or additions to tests label Nov 11, 2024
Copy link
Member Author

@sidvishnoi sidvishnoi left a comment

Choose a reason for hiding this comment

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

test-e2e

Copy link
Contributor

github-actions bot commented Nov 11, 2024

Extension builds preview

Name Link
Latest commit e60e377
Latest job logs Run #11855632168
BadgeDownload
BadgeDownload

) {
const url = getPopupUrl({ browserName, channel }, extensionId);
const url = await background.evaluate(() => chrome.action.getPopup({}));
Copy link
Member

Choose a reason for hiding this comment

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

Might it be worth to use browser from polyfill instead of chrome?

Copy link
Member Author

Choose a reason for hiding this comment

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

That'll require importing polyfill in each evaluate context.

Copy link
Member Author

@sidvishnoi sidvishnoi Nov 12, 2024

Choose a reason for hiding this comment

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

Will check if Playwright lets us define such polyfills (like globally). But chrome is supposed to work in Edge/Chrome and Firefox.

Copy link
Member Author

Choose a reason for hiding this comment

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

In most cases, something like const browser = globalThis.browser || globalThis.chrome should be enough (we don't need full polyfill).

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't find a way for now, but something like exposeBinding would be the way. Or we can make browser a global in background.js

Something of a general problem, I think I will check that as a followup.

Copy link
Member Author

@sidvishnoi sidvishnoi left a comment

Choose a reason for hiding this comment

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

test-e2e

Copy link
Member Author

@sidvishnoi sidvishnoi left a comment

Choose a reason for hiding this comment

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

test-e2e

Copy link
Member Author

@sidvishnoi sidvishnoi left a comment

Choose a reason for hiding this comment

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

test-e2e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: tests Improvements or additions to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants