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

✅ Custom Browser Event Tracker End-to-End Test #35354

Merged
merged 13 commits into from Aug 5, 2021

Conversation

kalemuw
Copy link
Contributor

@kalemuw kalemuw commented Jul 22, 2021

This PR is meant to add end to end testing for #35193.
Thus, it tests the custom browser event tracker functionality.

Reviewer: @alanorozco

@amp-owners-bot amp-owners-bot bot requested a review from avimehta July 22, 2021 14:33
@dmanek dmanek requested review from dmanek and removed request for avimehta July 26, 2021 14:36
extensions/amp-analytics/0.1/test-e2e/test-cbe.js Outdated Show resolved Hide resolved
extensions/amp-analytics/0.1/test-e2e/test-cbe.js Outdated Show resolved Hide resolved
extensions/amp-analytics/0.1/test-e2e/test-cbe.js Outdated Show resolved Hide resolved
extensions/amp-analytics/0.1/test-e2e/test-cbe.js Outdated Show resolved Hide resolved
extensions/amp-analytics/0.1/test-e2e/test-cbe.js Outdated Show resolved Hide resolved
extensions/amp-analytics/0.1/test-e2e/test-cbe.js Outdated Show resolved Hide resolved
extensions/amp-analytics/0.1/test-e2e/test-cbe.js Outdated Show resolved Hide resolved
extensions/amp-analytics/0.1/test-e2e/test-cbe.js Outdated Show resolved Hide resolved
@alanorozco alanorozco removed the request for review from kristoferbaxter August 4, 2021 19:58
@@ -384,6 +384,7 @@ const htmlFixtureGlobs = [
'!examples/xhr-intercept.html',
'!test/fixtures/e2e/amp-accordion/amp-accordion.html',
'!test/fixtures/e2e/amp-accordion/single-expand.html',
'!test/fixtures/e2e/amp-analytics/browser-events.html',
Copy link
Member

Choose a reason for hiding this comment

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

@rsimha This PR is currently blocked on approval from wg-approvers, which is a quite authoritative group.

What do you think about moving this glob set to a different file, so that we can set a more permissive list of OWNERS?

Copy link
Contributor

Choose a reason for hiding this comment

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

This change can currently be approved by anyone on infra or performance. Totally fine with adding other wgs to the list of owners.

{
owners: [
{name: 'ampproject/wg-infra'},
{name: 'ampproject/wg-performance'},
],
},

Copy link
Member

@alanorozco alanorozco Aug 5, 2021

Choose a reason for hiding this comment

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

My idea with extracting the .html example list is that modifying it is lower risk than other configuration objects in this file. I would feel more comfortable allowing a wider set of people to modify the .html list only. What do you think?

(In the meantime, your approval of this PR would be appreciated since you have wg-approvers powers that I don't. 🙂)

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea, for which there already is precedent 👍

Once the html list is moved to its own file, we can give everyone permissions to edit it, similar to this file:

{
pattern: 'dep-check-config.js',
owners: [{name: '*'}],
},

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

5 participants