-
Notifications
You must be signed in to change notification settings - Fork 2
Fix Playwright tests by adding more descriptive alt text to ad images #103
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
Conversation
packages/core/src/constants.ts
Outdated
export const DEFAULT_SERVICE_ENDPOINT: HTTPSURLString = IS_PRODUCTION | ||
? "https://ads.mozilla.org/" // production | ||
: "https://ads.allizom.org/" // staging | ||
: "https://ads.mozilla.org/" // staging |
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.
ehhh.. can we just update playwright to set NODE_ENV=production
instead?
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.
Yes, apologies for the mess, that would absolutely be the right way to do it, I will make that change. This was just me trying out a quick way to make sure this approach would work
packages/core/test/constants.test.ts
Outdated
|
||
test("Use staging endpoint URLs when not in production", () => { | ||
expect(DEFAULT_SERVICE_ENDPOINT).toBe("https://ads.allizom.org/") | ||
expect(DEFAULT_SERVICE_ENDPOINT).toBe("https://ads.mozilla.org/") |
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 isn't testing the right thing anymore.
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 point, thank you so much. Will fix.
d5a80de
to
f30911f
Compare
b213099
to
4933018
Compare
"example:iife": "npm run build && cd examples/iife && npm install && npm run dev", | ||
"example:react": "npm run build && cd examples/react && npm install && npm run dev", | ||
"example:iife": "npm run build:production && cd examples/iife && npm install && npm run dev", | ||
"example:react": "npm run build:production && cd examples/react && npm install && npm run dev", |
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.
Can we add example:iife:staging
and example:react:staging
scripts here to be able to also explicitly run the examples against staging (not as the default)
…e from placement name
04f54ab
to
563eccf
Compare
Before we have to resort to hard-coding these generic alt texts, can you check and see if we can just locate the
If this works for correctly identifying rendered ads in Playwright, we can avoid hard-coding the alt text and having to do a potentially-fragile mapping to the placement ID. Really, the alt text should come exclusively from MARS and our single generic "Mozilla Ad" text is really only intended as an absolute last resort. |
I haven't had a chance to try this but I think that would probably work, and there are lots of ways we can successfully locate and select these elements for test. But I think if we go this route, we are moving away from Playwright's best practice of using user-facing attributes. Writing our tests that way will actually give us more guarantees from Playwright under the hood that a real user can access and navigate the element on the page, in a way that selecting by a data attribute or test id would not. Playwright has a generator that will show you what the correct accessible locator is for an element. Using that, I see that the accessible locator for the ad should be But I think this exposes a bit of an accessibility issue. I think it would be better if the ads each had their own placement prefix, so the person using screen reader can just jump to the ad they mean without needing to loop through them all or thinking about their order on the page. But I'm not sure on this -- you also made a good point in our conversation about it a while back, like maybe the screen reader user doesn't care that it's a Billboard and that's not meaningful in the alt text? I'll make a post in #accessibility about this so they can make a general recommendation about the alt text, and what would be helpful for users on a pocket-like example page. There is also a ticket open for ads-eng/MARS which relates to "permanent canonical test placements" and specifies what we'd need for those to be "real enough" to consistently/repeatably drive a full suite of playwright e2e tests of the example apps. So, to summarize:
|
Been playing around with this for about a day now and as far as I can tell the MARS mock placements are back to behaving the same as they were before these started failing regularly (returning ads consistently, standard alt text, images being served). So I am going to close this PR and just re-enable the tests again. 🤞 |
Problem Statement
We need unique alt text on the ad images for our accessible-first Playwright tests to pass in the example apps, as that's the accessible way to select our ad elements.
Proposed Changes
We now construct alt text for the ad images on the client side in order to contextualize what kind of ad it is for users who aren't navigating visually. And we get the unique ids for the Playwright tests exactly as we need them.
Verification Steps
display.test.ts
incore
and make sure it's covering all the cases