Skip to content

E2E Auth: Add GitHub login test #103461

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Jun 13, 2025
Merged

E2E Auth: Add GitHub login test #103461

merged 18 commits into from
Jun 13, 2025

Conversation

bogiii
Copy link
Contributor

@bogiii bogiii commented May 15, 2025 •
edited
Loading

Part of https://linear.app/a8c/issue/DOTCOM-10590/fix-authentication-tests

Proposed Changes

  • Create GitHub authentication E2E test

Why are these changes being made?

  • Increases login variant coverage

Testing Instructions

  • CI: check the team city:
  • or locally
  • checkout this branch
  • run yarn workspace wp-e2e-tests build
  • run yarn workspace wp-e2e-tests test -- specs/authentication/authentication__github.ts
  • check if passes

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
    • For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@github-actions GitHub Actions
Copy link

github-actions bot commented May 15, 2025 •
edited by matticbot
Loading

Calypso Live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-147900
Jetpack Cloud live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-147900&env=jetpack
Automattic for Agencies live (direct link)
https://calypso.live?image=registry.a8c.com/calypso/app:build-147900&env=a8c-for-agencies

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@bogiii bogiii force-pushed the add/e2e-auth-github branch from 1ec6db1 to 60fdb9b Compare May 15, 2025 12:54
@bogiii bogiii self-assigned this May 15, 2025
@bogiii bogiii requested a review from a team May 15, 2025 13:01
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 15, 2025
@bogiii bogiii marked this pull request as ready for review May 15, 2025 13:02
@bogiii bogiii changed the title E2E Auth: Add github login test E2E Auth: Add GitHub login test May 15, 2025
github-advanced-security[bot]
github-advanced-security bot found potential problems May 15, 2025
View reviewed changes
github-advanced-security[bot]
github-advanced-security bot found potential problems May 15, 2025
View reviewed changes
github-advanced-security[bot]
github-advanced-security bot found potential problems May 15, 2025
View reviewed changes
test/e2e/specs/authentication/authentication__github.ts

it( 'Verify successful login to WordPress.com', async function () {
// Verify we're on WordPress.com
expect( page.url() ).toMatch( /.*wordpress\.com\/sites.*/ );

Check failure

Code scanning / CodeQL

Missing regular expression anchor

When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it.

Copilot Autofix

AI about 1 month ago

To fix the issue, we need to add anchors (^ and $) to the regular expression to ensure it matches the entire URL and not just a substring. Specifically, the regular expression /.*wordpress\.com\/sites.*/ should be updated to /^https:\/\/.*wordpress\.com\/sites.*$/. This ensures that the URL starts with https://, contains the expected domain and path, and matches the entire string.

Suggested changeset 1
test/e2e/specs/authentication/authentication__github.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/test/e2e/specs/authentication/authentication__github.ts b/test/e2e/specs/authentication/authentication__github.ts
--- a/test/e2e/specs/authentication/authentication__github.ts
+++ b/test/e2e/specs/authentication/authentication__github.ts
@@ -60,3 +60,3 @@
 			// Verify we're on WordPress.com
-			expect( page.url() ).toMatch( /.*wordpress\.com\/sites.*/ );
+			expect( page.url() ).toMatch( /^https:\/\/.*wordpress\.com\/sites.*$/ );
 		} );
EOF
@@ -60,3 +60,3 @@
// Verify we're on WordPress.com
expect( page.url() ).toMatch( /.*wordpress\.com\/sites.*/ );
expect( page.url() ).toMatch( /^https:\/\/.*wordpress\.com\/sites.*$/ );
} );
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
@bogiii bogiii marked this pull request as draft May 16, 2025 10:00
@paulopmt1
Copy link
Contributor

Do we still plan to move this one forward @bogiii?

@bogiii
Copy link
Contributor Author

bogiii commented Jun 9, 2025

Do we still plan to move this one forward @bogiii?

We are experiencing an issue with a suspended GH testing account. I'll try one more potential solution.

@bogiii bogiii force-pushed the add/e2e-auth-github branch from 22d9001 to 8069b8c Compare June 12, 2025 09:57
@bogiii bogiii marked this pull request as ready for review June 12, 2025 16:39
zaguiini
zaguiini approved these changes Jun 13, 2025
View reviewed changes
Copy link
Contributor

@zaguiini zaguiini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well. Left some suggestions, but no blockers!

packages/calypso-e2e/src/lib/pages/login-page.ts
* Clicks the "Continue with GitHub" link.
*/
async clickLoginWithGitHub(): Promise< Locator > {
const locator = await this.page.locator( ':text-is("Continue with GitHub")' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same here. I believe we can use page.getBy* matchers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entire file needs updates and selector replacements. I just matched the same approach.
I'll leave it as it is for now, and we can replace it in a separate PR.

test/e2e/specs/authentication/authentication__github.ts

it( 'Handle GitHub device verification if needed', async function () {
// GitHub may show a device verification screen in CI
const verificationUrl = 'https://github.com/sessions/verified-device';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can extract this to a constant? No biggie though.

@bogiii bogiii merged commit 7ba6766 into trunk Jun 13, 2025
9 of 10 checks passed
@bogiii bogiii deleted the add/e2e-auth-github branch June 13, 2025 15:04
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zaguiini zaguiini zaguiini approved these changes

Assignees

@bogiii bogiii

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bogiii @matticbot @paulopmt1 @zaguiini