Conversation
🦋 Changeset detectedLatest commit: d831b02 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| }; | ||
| }; | ||
|
|
||
| const makeSendServiceWorkerTrackingData = |
There was a problem hiding this comment.
In serviceWorker.js you already have a makeSendServiceWorkerTrackingData. Can you pass that to this module? When you need to create a new instance?
packages/core/src/components/Advertising/handlers/createOnBeforeSendEventHandler.js
Outdated
Show resolved
Hide resolved
packages/core/src/components/Advertising/handlers/createOnBeforeSendEventHandler.js
Show resolved
Hide resolved
|
The tests are still failing. |
|
I was thinking at this, and the refactoring seems to circumvent the errors but we still not know why is causing the tests to fail. We should probably still try to find out why the tests are flaky. |
I think they are failing because two or more tests import the same file, but one test mocks the file and the other does not. Our tests are run all in the same environment to use fewer resources on the CI environment. So the tests are order-dependent, I think. This PR removes the |
jonsnyder
left a comment
There was a problem hiding this comment.
Should we get the Ad-Tech contributors to look at your changes to the advertising component?
...ages/core/src/components/PushNotifications/helpers/serviceWorkerNotificationClickListener.js
Outdated
Show resolved
Hide resolved
I don't think that's necessary, since the changes are organizational, not functional |
|
What's up with the "fixme" notes? Is that things that were fixed or need to be fixed? |
Forgot about those. Fixed |
Description
During the creation of the
create-browser-packagebranch, I discovered a couple of patterns that were causing a number of instability in our integration and unit tests.This includes
fetchorquerySelectorThat weren't getting cleaned up.Because we run our tests in a non isolated and nonparallel manner on our continuous integration actions runners, this was causing the effects of one test to leak into others, and they would fail sometimes because the order was non deterministic.
Related Issue
Motivation and Context
Screenshots (if appropriate):
Checklist:
pnpm changeset) or it is not necessary because this PR is not consumer-facing.