[Issue #8184] allow usage of playwright tags in e2e test jobs#8960
[Issue #8184] allow usage of playwright tags in e2e test jobs#8960doug-s-nava merged 4 commits intomainfrom
Conversation
e854e8b to
edb8166
Compare
f992a5f to
a9ec3b6
Compare
a9ec3b6 to
9636b66
Compare
.github/actions/e2e/action.yml
Outdated
| npx playwright install --with-deps | ||
| - name: Run e2e tests (Shard ${{ inputs.shard }}/${{ inputs.total_shards }}) | ||
| - name: Run all e2e tests (Shard ${{ inputs.shard }}/${{ inputs.total_shards }}) |
There was a problem hiding this comment.
................inputs.current_shard }}/${{ inputs.total_shards }})
Missing -current-?
.github/actions/e2e/action.yml
Outdated
| shell: bash | ||
|
|
||
|
|
||
| - name: Run e2e tests for ${{ inputs.playwright_tags }} (Shard ${{ inputs.shard }}/${{ inputs.total_shards }}) |
There was a problem hiding this comment.
name: Run e2e tests for ${{ inputs.playwright_tags }} (Shard ${{ inputs.current_shard }}/${{ inputs.total_shards }})
This line also missing -current-
| description: "pipe separated list of @tags denoting groups of tests to run" | ||
| required: false | ||
| type: string | ||
| workflow_call: |
There was a problem hiding this comment.
Maybe add these as well?
inputs:
api_logs:
description: "print api logs on failure?"
default: "false"
type: string
playwright_tags:
description: "pipe separated list of @tags denoting groups of tests to run"
required: false
type: string
| type: choice | ||
| options: | ||
| - "true" | ||
| - "false" |
There was a problem hiding this comment.
I will use boolean instead:
default: false
type: boolean
then update the below :
api_logs: ${{ inputs.api_logs }} to be api_logs: ${{ inputs.api_logs && 'true' || 'false' }}
.github/workflows/e2e-staging.yml
Outdated
| @@ -37,12 +41,14 @@ jobs: | |||
| - name: Set target | |||
| run: if [[ -z "${{ inputs.target }}" ]]; then echo "defaulted_target=staging" >> "$GITHUB_ENV"; else echo 'defaulted_target="${{ inputs.target }}"' >> "$GITHUB_ENV"; fi | |||
There was a problem hiding this comment.
I know it is not new changes but this seem weird:
maybe fix the ' and "?
run: if [[ -z "${{ inputs.target }}" ]]; then echo "defaulted_target=staging" >> "$GITHUB_ENV"; else echo "defaulted_target=${{ inputs.target }}" >> "$GITHUB_ENV"; fi
frontend/tests/e2e/index.spec.ts
Outdated
|
|
||
| test("has title", async ({ page }) => { | ||
| test("has title", { tag: "@smoke" }, async ({ page }) => { | ||
| await expect(page).toHaveTitle(/Simpler.Grants.gov/); |
| /https:\/\/github.com\/HHS\/simpler-grants-gov/, | ||
| ); | ||
| }); | ||
| test( |
There was a problem hiding this comment.
I will do this:
test(
"clicking 'follow on GitHub' link opens a new tab pointed at Github repository",
{ tag: "@full-regression" },
async ({ page, context }) => {
const pagePromise = context.waitForEvent("page");
// Click the Follow on GitHub link
await page.getByRole("link", { name: "Follow on GitHub" }).click();
const newPage = await pagePromise;
await expect(newPage).toHaveURL(
/^https://github.com/HHS/simpler-grants-gov(?:/)?$/,
);
},
);
There was a problem hiding this comment.
just to confirm, the change here is to change the regex from /https:\/\/github.com\/HHS\/simpler-grants-gov/ to /^https://github.com/HHS/simpler-grants-gov(?:/)?$/, or is there something else?
There was a problem hiding this comment.
We should escape the period, but with escaping, I think ^https:\/\/github\.com\/HHS\/simpler-grants-gov(?:\/)\?$ is what you're proposing, and I'm not sure how that is functionally any different from /https:\/\/github\.com\/HHS\/simpler-grants-gov in this case (where we're assuming that the protocol of the url will come at the beginning)
There was a problem hiding this comment.
| test( | |
| test( | |
| "clicking 'follow on GitHub' link opens a new tab pointed at Github repository", | |
| { tag: "@full-regression" }, | |
| async ({ page, context }) => { | |
| const pagePromise = context.waitForEvent("page"); | |
| // Click the Follow on GitHub link | |
| await page.getByRole("link", { name: "Follow on GitHub" }).click(); | |
| const newPage = await pagePromise; | |
| await expect(newPage).toHaveURL( | |
| /^https:\/\/github\.com\/HHS\/simpler-grants-gov(?:\/)?$/, | |
| ); | |
| }, | |
| ); |
There was a problem hiding this comment.
I just learned that I have to put the 'add a suggestion' to put code in comment. This is what I type.
frontend/tests/e2e/index.spec.ts
Outdated
| const newPage = await pagePromise; | ||
| await newPage.waitForLoadState(); | ||
| await expect(newPage).toHaveURL( | ||
| /^https:\/\/github\.com\/HHS\/simpler-grants-gov/, |
There was a problem hiding this comment.
This one will allow:
https://github.com/HHS/simpler-grants-gov/issues
https://github.com/HHS/simpler-grants-gov?x=1
Anything that starts with that prefix
This one only allows:
| /^https:\/\/github\.com\/HHS\/simpler-grants-gov/, | |
| /^https:\/\/github\.com\/HHS\/simpler-grants-gov(?:\/)?$/, |
https://github.com/HHS/simpler-grants-gov
https://github.com/HHS/simpler-grants-gov/
There was a problem hiding this comment.
got it - this does make this more specific. At this point it should probably just be a string though. I'll make that change
|
Reviewed the changes in this PR and verified that they work as intended. |

Summary
Fixes for #8184
Changes proposed
Adds support for limiting which playwright tests are run in our e2e testing ci jobs by passing playwright tags.
Context for reviewers
Includes definition of groups to two tests - these are meant to facilitate verification and manual testing of this PR, but the assigned groups are also the correct groups for these, so leaving them in the PR.
Also cleans up the implementation of the "api_logs" param in the local ci job, which was previously accepting free text
Validation steps
If you'd like to test this change yourself, feel free to add the experimental job back and play around with it