Dr 3790/ Fixing George Arents division tests for localhost#466
Conversation
* Arents division now uses route-filters * Alphabetize routes list * Add New Relic and more Adobe routes * Change pagination to isVisible for Arents
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
samanthaandrews
left a comment
There was a problem hiding this comment.
Let's figure out a way to abort these scripts for all tests instead of doing them on every page separately.
Also, make sure to add the Jira link to the description.
| // Block analytics, tracking, and third-party domains | ||
| await page.route(/.*adobedc\.net.*/, (route) => route.abort()); | ||
| await page.route(/.*adobedtm\.com.*/, (route) => route.abort()); | ||
| await page.route(/.*demdex\.net.*/, (route) => route.abort()); | ||
| await page.route(/.*everesttech\.net.*/, (route) => route.abort()); | ||
| await page.route(/.*google-analytics\.com.*/, (route) => route.abort()); | ||
| await page.route(/.*google\.com.*/, (route) => route.abort()); | ||
| await page.route(/.*googletagmanager\.com.*/, (route) => route.abort()); | ||
| await page.route(/.*ipify\.org.*/, (route) => route.abort()); | ||
| await page.route(/.*newrelic\.com.*/, (route) => route.abort()); | ||
| await page.route(/.*nr-data\.net.*/, (route) => route.abort()); | ||
| await page.route(/.*omappapi\.com.*/, (route) => route.abort()); |
There was a problem hiding this comment.
Since these aborts are already implemented as part of #465, which has been merged to the base branch, we should instead make these part of a script that runs before all of the playwright tests (https://playwright.dev/docs/test-global-setup-teardown). It's likely that these scripts run on every page in DC and we should stick to the "don't repeat yourself" principle to avoid unnecessary duplication that will be difficult to maintain.
There was a problem hiding this comment.
Moved these filters into the config.
Getting tests to pass on Arents pagination (or lack thereof) now. Turns out it depends on whether you're hitting qa-api or prod-api from within the test-dc site. So maybe we need either a different Division to test (that always has pagination in both environments) or visa versa a division to test that is always a single page, if we're also testing for that conditional behavior. Arents sits between these two conditions ... uncomfortably.
| }, | ||
| { auto: true }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
Usually we want to keep the config file for playwright configs only and move these utils in a separate file.
There was a problem hiding this comment.
Yeah let's move this into a separate utils or fixtures directory. Maybe something like playwright/fixtures/base.ts.
// fixtures/base.ts
import { test as base } from '@playwright/test';
export const test = base.extend({
page: async ({ page }, use) => {
// Set up route blocking before each test
await page.route('**/*.{png,jpg,jpeg,gif,webp,svg}', route => route.abort());
await page.route('**/analytics/**', route => route.abort());
await page.route('**/ads/**', route => route.abort());
await use(page);
},
});
export { expect } from '@playwright/test';
Then, in every test that uses this fixture, it will need to import this test and expect object:
// tests/example.test.ts
import { test, expect } from '../fixtures/base';
// rest of the test
Or I believe you can do what you did below and add the ...test object to the Chromium project below.
There was a problem hiding this comment.
I've added a /playwright/utils dir that holds data/conditions for global filters, etc. There is one file in that dir currently named routeFilters.ts. I put the global route-filtering logic in a new base.ts that lives at the root of /playwright. For a test to use the current fixture in base.ts, it will need to have this line at top:
// tests/example.test.ts
import { test, expect } from '../base';
The only config in playwright.config.ts now is the tag declaration for @no-global-filter which allows an override of the filter call within any test. Since there might be tests where we don't want to filter out those 3rd-party metrics occasionally. The tag in those cases can be added on the command-line or in CI, and the filter won't be appended.
I've only added the '../base' import to the homepage and george-arent-division-landing tests for this PR. Once this work is stable and doesn't need further revisions, I'll add that import ... from '.../base' line to the rest of our test-files.
samanthaandrews
left a comment
There was a problem hiding this comment.
For readability let's move the route filter fixture into a separate file.
| }, | ||
| { auto: true }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
Yeah let's move this into a separate utils or fixtures directory. Maybe something like playwright/fixtures/base.ts.
// fixtures/base.ts
import { test as base } from '@playwright/test';
export const test = base.extend({
page: async ({ page }, use) => {
// Set up route blocking before each test
await page.route('**/*.{png,jpg,jpeg,gif,webp,svg}', route => route.abort());
await page.route('**/analytics/**', route => route.abort());
await page.route('**/ads/**', route => route.abort());
await use(page);
},
});
export { expect } from '@playwright/test';
Then, in every test that uses this fixture, it will need to import this test and expect object:
// tests/example.test.ts
import { test, expect } from '../fixtures/base';
// rest of the test
Or I believe you can do what you did below and add the ...test object to the Chromium project below.
* remove route-filters from playwright.config.ts * add grepInvert option for global-route-filters tag * correct flakey-test syntax for george-arents-division pagination skip * call test object from base on homepage and george-arents-division tests * add specific route-filters to a utils file * put global route-filters logic and tag-based override in base file
| "verify pagination is present and visible", | ||
| { tag: "@flaky" }, | ||
| async ({ page }) => { | ||
| test.skip(); |
There was a problem hiding this comment.
nit: you can move this to line 27, i.e. test.skip("verify pagination is present and visible")
clarissarichard
left a comment
There was a problem hiding this comment.
This is awesome work!
| // If necessary, block the main-image overlay from iiif. | ||
| // When running the whole suite, feedback on the homepage will often | ||
| // timeout when default img overlays are slow | ||
| await page.route("**/default.jpg", (route) => route.abort()); |
There was a problem hiding this comment.
Just out of curiosity, should default.jpg also be included in applyRouteFilters? Or will it interfere with tests checking the visibility of certain images?
There was a problem hiding this comment.
I left it out of the global filters, because I'm not sure that there could be cases where we do want to load/see the default image. And I've mainly seen the failure of the default jpg affecting the homepage. So I left it as a local per-file option, but we can definitely revisit if it turns out it's useful to have in the global filters.
Ticket:
This PR does the following:
Added a few small changes, based on the previous localhost/timeouts work.
Adds more route-filters to both homepage and arents-division tests, including more New Relic and Adobe routes.
In testing the localhost timing, I ran into the curious case where the George Arents division pages tests on pagination are failing on localhost but were passing on the CI. At first I thought it was a timing issue, but .... (DISREGARD: have found also that the pagination locator should be finding a visible page "1.") So I've adjusted that test accordingly.
I also alphabetized the route-filters to more easily keep track of what we're filtering.
UPDATE:
Well, revised tests ended up failing for the pagination locator in CI. So we have a situation were they pass on ci and fail on localhost, and when pagination is flipped to isVisible, they fail on ci and pass on localhost. I think this points to a viewport or similar issue, so for now I'm going to switch the locator for pagination back to isHidden, until I can get a reasonable fix which is correct both on ci and localhost.
This PR should still be updating the route-filters.
UPDATE-2:
Route-filters have been moved into config for the 3rd-party metrics, etc.
Pagination fails between CI and localhost turn out to be based on whether your local env is pointed to prod-api or qa-api. Given separation of concerns, I had been pointing at prod-api in my localhost test-runs for DCFL. While the CI was pointing at qa-api by default. Arents Division has a different number of collections (smaller on QA) in the two settings, and pagination turns out to be dependent on that difference (currently 36 collections in QA, and 54-ish collections on PROD)..
Open questions
I haven't investigated why the CI seems to get a "false positive" on the "isHidden" for the pagination, but one possibility is a smaller than regulation viewport in the CI. This is worth investigating further to avoid other false-positives like this for locators that are trying to assert that something is hidden. There are also some Playwright locator options for scrolling, etc that might also help. But for this specific test, since it should really be looking for an "isVisible," I don't think we really need to investigate further in this PR.
UPDATE: see previous section.
In a future PR I will also be putting the route-filters into a call for all the tests.
How has this been tested? How should a reviewer test this?
I ran the tests locally and analyzed the network console for Playwright in the UI to make sure I've picked up all of the relevant 3rd party route-filters needed for localhost.
Accessibility concerns or updates
Checklist: