-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Remove noisy 3p error for user error reporting #11382
Conversation
3p/integration.js
Outdated
@@ -444,7 +450,6 @@ window.draw3p = function(opt_configCallback, opt_allowed3pTypes, | |||
try { | |||
const data = getAttributeData(); | |||
const location = getLocation(); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no need to remove this line
} | ||
} | ||
</script></amp-analytics>`, | ||
}, env => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: please +2 indent string body
801b8a4
to
14b2db8
Compare
</script></amp-analytics>`, | ||
}, env => { | ||
it('should ping correct host with 3p error message', () => { | ||
return expect(withdrawRequest(env.win, randomId)).to.eventually.be.ok; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chai-as-promised
assertions aren't available for integration tests. See test/chai-as-promised/chai-as-promised.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, you still can use regular chai assertions. So the ones in http://chaijs.com/api/bdd/ are available in integration tests, but the ones in https://github.com/domenic/chai-as-promised are not available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rsimha-amp I fixed the test by return a promise w/o using chai assertions.
I found that expect(promise).to.be.ok
will always return true. is it true that it equals to expect(promise).to.be.defined
? Asking because I see we use expect(promise).to.be.ok
else where, want to make sure we check the correct things in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to.be.defined
is invalid. See my recent email to the team :)
Seems like this is broken on |
* remove noisy 3p error, add integration test * try to fix test * nit fix * fix integration test
…o ke/amp-addthis * 'ke/amp-addthis' of https://github.com/dmvjs/amphtml: (62 commits) type definitions to appease GCC type definitions to appease GCC type definitions to appease GCC type definitions to appease GCC type definitions to appease GCC type definitions to appease GCC type definitions to appease GCC test fixes Add required logic for view/engagement analytics without an iframe Lojson call refactor; saving WIP Code review cleanup Make all images in AMP have the `async` attribute. (ampproject#11366) skip test (ampproject#11459) Fix having multiple Criteo standalone ads on one page (ampproject#11285) add `bt` (binary type) query param to error reporting query string (ampproject#11452) Enables vendor iframe to send response to creative (ampproject#11306) Remove noisy 3p error for user error reporting (ampproject#11382) Ramp Fast Fetch crypto verifier experiment to 10% (ampproject#11447) Updates doubleclick impl to include docs on fluid (ampproject#11437) Revert "Add a visual diff test for amp-pinterest" (ampproject#11442) ...
…o jr/amp-addthis * 'jr/amp-addthis' of https://github.com/dmvjs/amphtml: (54 commits) Add required logic for view/engagement analytics without an iframe Lojson call refactor; saving WIP Code review cleanup Make all images in AMP have the `async` attribute. (ampproject#11366) skip test (ampproject#11459) Fix having multiple Criteo standalone ads on one page (ampproject#11285) add `bt` (binary type) query param to error reporting query string (ampproject#11452) Enables vendor iframe to send response to creative (ampproject#11306) Remove noisy 3p error for user error reporting (ampproject#11382) Ramp Fast Fetch crypto verifier experiment to 10% (ampproject#11447) Updates doubleclick impl to include docs on fluid (ampproject#11437) Revert "Add a visual diff test for amp-pinterest" (ampproject#11442) Ramp experiment down to 0 (ampproject#11441) Updated changelog with recent validator release (ampproject#11440) Remove pfx parameter from Doubleclick Fast Fetch reuests (ampproject#11439) Allow element to accept custom share attributes (with fallback); remove other ownerDocument refs Add wait code for amp.article.html (ampproject#11429) fix inabox-viewport test (ampproject#11428) Remove camelcase from data attributes Amp Ad Fast Fetch allow amp-image prefetch (ampproject#11391) ...
…o jr/amp-addthis * 'jr/amp-addthis' of https://github.com/dmvjs/amphtml: (54 commits) Add required logic for view/engagement analytics without an iframe Lojson call refactor; saving WIP Code review cleanup Make all images in AMP have the `async` attribute. (ampproject#11366) skip test (ampproject#11459) Fix having multiple Criteo standalone ads on one page (ampproject#11285) add `bt` (binary type) query param to error reporting query string (ampproject#11452) Enables vendor iframe to send response to creative (ampproject#11306) Remove noisy 3p error for user error reporting (ampproject#11382) Ramp Fast Fetch crypto verifier experiment to 10% (ampproject#11447) Updates doubleclick impl to include docs on fluid (ampproject#11437) Revert "Add a visual diff test for amp-pinterest" (ampproject#11442) Ramp experiment down to 0 (ampproject#11441) Updated changelog with recent validator release (ampproject#11440) Remove pfx parameter from Doubleclick Fast Fetch reuests (ampproject#11439) Allow element to accept custom share attributes (with fallback); remove other ownerDocument refs Add wait code for amp.article.html (ampproject#11429) fix inabox-viewport test (ampproject#11428) Remove camelcase from data attributes Amp Ad Fast Fetch allow amp-image prefetch (ampproject#11391) ...
Collecting every 3p iframe error can bring so much noise to error reporting.
Earlier @cramforce mentioned that publisher can collect those errors and request ad networks to fix them. However this is not a good time to do this, as we are still promoting this new error reporting feature.
I removed all noisy 3p errors. We can consider report them after publishers have widely adopted this feature. Or we can introduce a flag later so that one can specify if 3p errors reporting are wanted or not.
Integration test also added.
to @alanorozco since @lannka is out : )
cc/ @cramforce