test(functional): Convert Robots test, oauth webchannel tests #15379

LZoog · 2023-05-26T00:22:39Z

Because:

We're converting tests from intern to playwright

This commit:

Converts oauth_webchannel.js, robots.txt tests
Does not port over ios v1 sign up since we don't need to support it any longer
Tweaks fillOutFirstSignUp to give it an options argument

Previous notes below.

Struggling a bit with these oauth webchannel tests, just need to get them passing and then I'll clean them up, any assist here is welcomed. 😬

In 5d21ba8, I can see the event received here but then it's not picked up in the oauth web channel broker in fxaStatus.

I then tried just doing a copy-over of what we're doing in content-server in a872784, e.g. going to 123done and going through email-first with the context param, then hitting respondToWebChannelMessage. It finds the newly created attached div, but I don't even see the event in web-channel.js or fxaStatus.

Since we're not receiving the event in fxaStatus to update capabilities, _getSyncEngines in CWTS fails, causing a 500.

I originally tried some things with checkWebChannelMessage with no luck, but maybe I'm missing something. I'll try a combination of this instead of a listener with dispatching the response, but after that I'll probably have hit a wall here.

Will close FXA-5923

ashrivastava-qa · 2023-06-01T13:00:24Z

@LZoog I tried to make some changes to your branch to see if I can get it working but with the bare minimum experience I have had working with web-channels so far wasn't fruitful. I am stumped too on this. We should probably reach out to @vbudhram or @chenba for help on this!

LZoog · 2023-06-09T01:19:26Z

packages/functional-tests/lib/channels.ts

+export enum FirefoxCommand {
+  FxAStatus = 'fxaccounts:fxa_status',
+  OAuthLogin = 'fxaccounts:oauth_login',
+  Logout = 'fxaccounts:logout',


I tried using this in a couple other tests and ran into some type issues with createCustomEventDetail so it might need tweaking but I think this works for now.

We may want to move these from settings into a shared file.

LZoog · 2023-06-09T01:21:05Z

packages/functional-tests/pages/layout.ts

+          const command = detail.message.command;
+          const messageId = detail.message.messageId;
+
+          if (command === expectedCommand) {


@vbudhram I was able to remove the attached div stuff and it passes every time for me, it felt OK to remove. I can also remove some of this if (command === expectedCommand) { logic and it works, but, I'm not sure we should strip that out yet because I think we may need it in other tests if we need to wait for some event that doesn't fire immediately.

LZoog · 2023-06-09T01:21:24Z

packages/functional-tests/pages/login.ts

@@ -50,6 +50,9 @@ export const selectors = {
  NOT_EMAIL_MET: '#password-same-as-email.password-strength-met',
  NOT_EMAIL_FAIL: '#password-same-as-email.password-strength-fail',
  PERMISSION_ACCEPT: '#accept',
+  CWTS_HEADER: 'text="Choose what to sync"',


If we start using text selectors here where we can, it's less work when we get to the React conversion work.

LZoog · 2023-06-09T01:22:28Z

packages/functional-tests/pages/login.ts

    await this.page.locator(selectors.SUBMIT).click();
-    return waitForNavigation;
+    // return waitForNavigation;


Wanted to see if removing this made stuff fail (having trouble running them all locally), for some reason when this is left my new tests don't pass. It just hangs after clicking submit on the account confirmation page. If there's a bunch of failures this'll be the cause and I'll tweak.

edit: yeah, things be failing. Looking now.

Did we make the change here so that all the failing tests works now? @LZoog

Yes that's right! See this comment.

Whenever there's more than a couple of parameters passed like we were doing, an options object can start to feel a bit nicer so I did go ahead and have to make those changes across files.

LZoog · 2023-06-09T01:24:28Z

packages/functional-tests/pages/login.ts

@@ -355,6 +358,24 @@ export class LoginPage extends BaseLayout {
    return this.page.locator(selectors.SUBMIT).click();
  }

+  async waitForCWTSHeader() {
+    await this.page.waitForSelector(selectors.CWTS_HEADER, {


I don't think our other tests with this...

return this.page.isVisible(selectors.SYNC_CONNECTED_HEADER, { timeout: 100, });

... actually wait. It looks like isVisible executes immediately, even if I slapped a 5000 second timeout it wouldn't wait. Personally I think simply using this.page.waitForSelector in our selector checks and then a simple await waitForCWTSHeader(); call is nice over checking for truthiness anyway; if it doesn't find the element the test fails.

I'm onboard to make this change 👍🏽

LZoog · 2023-06-09T01:24:55Z

packages/functional-tests/tests/misc.spec.ts

@@ -28,3 +28,13 @@ test.describe('severity-1', () => {
    expect(await relier.isLoggedIn()).toBe(true);
  });
 });
+
+test.describe('robots.txt', () => {


I'd written this one a while ago, I can remove if we no longer want this test but we don't have any unit tests for it so it seems good to leave to me?

We can leave it on here!

Because: * We're converting tests from intern to playwright This commit: * Converts oauth_webchannel.js, robots.txt tests * Does not port over ios v1 sign up since we don't need to support it any longer * Tweaks fillOutFirstSignUp to give it an options argument Closes FXA-5923

ashrivastava-qa

LGTM, added just one question about the failing tests you mentioned. Also since you tweaked the fillOutFirstSignUp(), I am guessing we made the changes on all the files that's using the function (I do see a lot of files where you have already made the changes but just wanted to double check :)). I will wait for @vbudhram to approve this though. Thanks @LZoog for working on this!

ashrivastava-qa · 2023-06-12T14:52:24Z

packages/functional-tests/tests/misc.spec.ts

@@ -28,3 +28,13 @@ test.describe('severity-1', () => {
    expect(await relier.isLoggedIn()).toBe(true);
  });
 });
+
+test.describe('robots.txt', () => {


We can leave it on here!

ashrivastava-qa · 2023-06-12T14:54:16Z

packages/functional-tests/pages/login.ts

    await this.page.locator(selectors.SUBMIT).click();
-    return waitForNavigation;
+    // return waitForNavigation;


Did we make the change here so that all the failing tests works now? @LZoog

vbudhram · 2023-06-14T15:57:50Z

packages/functional-tests/pages/login.ts

-  async submit() {
-    const waitForNavigation = this.page.waitForNavigation();
+  async submit(waitForNav = true) {
+    if (waitForNav) {


Would you be able to use waitForUrl and have it accept a RegEx of possible paths? I haven't seen these waitForNavigation errors pop up in a while though.

Looks like we should be able to and that it's the recommendation now:

I could change this for the one test, but I think that might be better in a follow up since we'd still need some conditional here until we do it across the board for all tests that use this - it looks like we call this.submit in login.ts for various things like setTotp and setNewPassword etc. so we need to pass the expected URL regex in there when we call this.submit.

vbudhram · 2023-06-14T16:16:42Z

packages/functional-tests/tests/misc.spec.ts

+test.describe('robots.txt', () => {
+  test('should allow bots to access all pages', async ({ target, page }) => {
+    await page.goto(`${target.contentServerUrl}/robots.txt`, {
+      waitUntil: 'networkidle',


From latest docs 😅 -networkidle - **DISCOURAGED** consider operation to be finished when there are no network connections for at least 500 ms. Don't use this method for testing, rely on web assertions to assess readiness instead.

Not for this PR but good for follow up.

vbudhram

@LZoog Thank you!

LZoog · 2023-06-15T22:02:16Z

Filed follow ups mentioned here:

LZoog force-pushed the FXA-5923 branch 2 times, most recently from a21899a to 16454ce Compare June 9, 2023 01:17

LZoog commented Jun 9, 2023

View reviewed changes

LZoog marked this pull request as ready for review June 9, 2023 01:26

LZoog requested a review from a team as a code owner June 9, 2023 01:26

LZoog force-pushed the FXA-5923 branch from 16454ce to 3b472a3 Compare June 9, 2023 16:16

LZoog force-pushed the FXA-5923 branch from 3b472a3 to 880d9c5 Compare June 9, 2023 17:11

ashrivastava-qa reviewed Jun 12, 2023

View reviewed changes

vbudhram reviewed Jun 14, 2023

View reviewed changes

vbudhram approved these changes Jun 14, 2023

View reviewed changes

LZoog merged commit db56b87 into main Jun 15, 2023

LZoog deleted the FXA-5923 branch June 15, 2023 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(functional): Convert Robots test, oauth webchannel tests #15379

test(functional): Convert Robots test, oauth webchannel tests #15379

LZoog commented May 26, 2023 •

edited

Loading

ashrivastava-qa commented Jun 1, 2023

LZoog Jun 9, 2023

LZoog Jun 9, 2023

LZoog Jun 9, 2023

LZoog Jun 9, 2023 •

edited

Loading

ashrivastava-qa Jun 12, 2023

LZoog Jun 13, 2023

LZoog Jun 9, 2023 •

edited

Loading

vbudhram Jun 14, 2023

LZoog Jun 9, 2023

ashrivastava-qa Jun 12, 2023

ashrivastava-qa left a comment

ashrivastava-qa Jun 12, 2023

ashrivastava-qa Jun 12, 2023

vbudhram Jun 14, 2023

LZoog Jun 15, 2023

vbudhram Jun 14, 2023

vbudhram left a comment

LZoog commented Jun 15, 2023

test(functional): Convert Robots test, oauth webchannel tests #15379

test(functional): Convert Robots test, oauth webchannel tests #15379

Conversation

LZoog commented May 26, 2023 • edited Loading

ashrivastava-qa commented Jun 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LZoog Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LZoog Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashrivastava-qa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbudhram left a comment

Choose a reason for hiding this comment

LZoog commented Jun 15, 2023

LZoog commented May 26, 2023 •

edited

Loading

LZoog Jun 9, 2023 •

edited

Loading

LZoog Jun 9, 2023 •

edited

Loading