Surrogates test #32
Surrogates test #32
Conversation
jdorweiler
commented
Mar 4, 2021
jdorweiler
commented
There was a problem hiding this comment.
Great job with the styling and use of all those ancient table APIs ❤️ That being said, please fix the linting, because
┏┓
┃┃╱╲ In this
┃╱╱╲╲ house
╱╱╭╮╲╲ we love
▔▏┗┛▕▔ & appreciate
╱▔▔▔▔▔▔▔▔▔▔╲
Semicolons
╱╱┏┳┓╭╮┏┳┓ ╲╲
▔▏┗┻┛┃┃┗┻┛▕▔
I also left some other comments for your enjoyment.
| } | ||
| </style> | ||
| </head> | ||
| <body onload="loadSurrogates()"> |
There was a problem hiding this comment.
can we move this to script to make the linter happy?
|
|
||
| const table = document.getElementById('results-table') | ||
| const row = table.insertRow(-1) | ||
| const testName = row.insertCell(0) |
There was a problem hiding this comment.
TIL about .insertRow and .insertCell. I'm learning!
| cleanUp: () => { delete window._gaq } | ||
| }, | ||
| 'Directly accessing a web resouce' : { | ||
| url: "chrome-extension://lfpgfgegioonagopmelghcelfgffmjnh/web_accessible_resources/analytics.js", |
There was a problem hiding this comment.
wait, what extension ID is this? Doesn't look like ours: https://chrome.google.com/webstore/detail/duckduckgo-privacy-essent/bkdgflcldnnnapblkhphbgpggdiikppg ?
| } | ||
| } | ||
|
|
||
| function updateTable ({name, testData, error}) { |
There was a problem hiding this comment.
can we also have a global object with test results like in other tests? This way, when testing this page automatically, you will not need to scrape HTML, you'll just pull info from window.results.
| s.crossOrigin = testData.crossOrigin; | ||
| } | ||
|
|
||
| s.src = testData.url; |
There was a problem hiding this comment.
I remember that in some cases (local files? cache?) event could fire before listener is set up if you provide url first. Just to be extra sure - can you move this after listeners are ready?
| url: 'https://google-analytics.com/analytics.js', | ||
| crossOrigin: 'anonymous', | ||
| notes: 'Test loading with crossOrigin set on element (should fail on Firefox) https://bugzilla.mozilla.org/show_bug.cgi?id=1694679', | ||
| test: () => { return window.ga.answer === 42; }, |
There was a problem hiding this comment.
why are we testing it this way and not the same way as _gaq?
There was a problem hiding this comment.
Good point, I learned this would return true with the real ga code too. I updated it to look for an empty object from ga.create()
| 'google-analytics.com/analytics.js': { | ||
| url: 'https://google-analytics.com/analytics.js', | ||
| test: () => { return window.ga.answer === 42; }, | ||
| cleanUp: () => { delete window.ga; } |
There was a problem hiding this comment.
we are not waiting for test to load/fail, so all tests are running at once - can there be a race condition when they overlap (e.g. two tests use window.ga)?
There was a problem hiding this comment.
Thanks, fixed it 👀
| test: () => { return window.ga.answer === 42; }, | ||
| cleanUp: () => { delete window.ga; } | ||
| }, | ||
| 'google-analytics, ga.js': { |
There was a problem hiding this comment.
all other tests involve testing different configurations of analytics.js surrogate why are we also testing ga.js surrogate? Is this some kind of a special case?
There was a problem hiding this comment.
No, not special, basically the same as analytics. I got rid of it.
|
Thanks a lot for answering all my questions! |
