-
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
🐛Add computed styles to fake win #26514
Conversation
cc/ @rsimha who might have ideas about how to prevent things like this in the future. |
|
||
// Styles. | ||
this.getComputedStyle = function() { | ||
return window.getComputedStyle.apply(window, arguments); |
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.
Good find. Could it be that you want
arguments[0].defaultView
instead of window
?
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.
Maybe :) arguments[0]
is the element here, so I tried arguments[0].ownerDocument.defaultView
which points at the fake win, but it throws an Illegal invocation error.
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.
This would be expected to work when the element is from the actual window.
I believe you hit #15658.
This is a long standing issue that I've investigated from time to time. One thing I haven't explored yet, but mean to, is to check if your test suite is doing something asynchronous inside a Let me know if you spot something in your tests that matches this antipattern. |
…frame-pymjs-support * 'master' of github.com:ampproject/amphtml: (436 commits) 🐛Add computed styles to fake win (ampproject#26514) 📦 Update dependency eslint-config-prettier to v6.10.0 (ampproject#26518) 🐛 Rewrite relative urls in the bookend (ampproject#26490) ✨ [amp-list] Introduce "amp-state:" as a usable protocol for the src attribute. (ampproject#26284) Add terminal newline to files.txt (ampproject#26513) 📦 Update dependency chromedriver to v79.0.2 (ampproject#26515) ♻️ Change the opt-in cookie values to reflect upcoming CDN changes (ampproject#26489) ♻️<amp-next-page> v2 minor renaming and fixes (ampproject#26468) fix link (ampproject#26508) amp-list: fix broken test re. missing layout (ampproject#26509) performance: emit timeOrigin w/ polyfill (ampproject#26485) Position n+1 story desktop page before positioning attributes are set. (ampproject#26488) ✨amp-nested-menu: allow svg into (ampproject#26502) Log user warning when missing URL arg for shadow doc (ampproject#26290) 📦 Update dependency puppeteer to v2.1.0 (ampproject#26501) Ensure that fluid slots hidden by media queries do not issue ad requests. (ampproject#26352) 📦 Update dependency mocha to v7.0.1 (ampproject#26495) 📦 Update dependency fetch-mock to v8.3.2 (ampproject#26491) Revert 'Move mutator implementations out to a standalone service' (ampproject#26479) Fix syntax error (ampproject#26481) ...
While investigating test failures for #26447, I noticed that I would get a different number of tests each time when running
test-runtime
locally?!?After investigation it appears many (all?) of the tests using
fakeWin
in this file would throw an error when the runtime eventually callsgetComputedStyle
. This PR adds this method to the fake win.I think this was causing all sorts of weird behaviors, and likely that many tests were unintentionally being skipped due to the test runner borking. When I added the method it surfaced two obvious errors that must not have been run all this time :(
Also might be opening a can of worms here by exposing other uncaught errors in other test files that may be exposed after introduction of this method onLooks like tests passed.fakeWin
. Going to let travis run and see what happens.