Skip to content
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

RequestHook regressions with native automation #7640

Closed
Klaster1 opened this issue Apr 19, 2023 · 16 comments · Fixed by #7712
Closed

RequestHook regressions with native automation #7640

Klaster1 opened this issue Apr 19, 2023 · 16 comments · Fixed by #7712
Assignees
Labels
SYSTEM: native automation TYPE: bug The described behavior is considered as wrong (bug).

Comments

@Klaster1
Copy link

Klaster1 commented Apr 19, 2023

What is your Scenario?

All my tests run against a fake service which I redirect requests to with a RequestHook.

What is the Current behavior?

  1. The hook does not intercept WebSocket handshakes.
  2. Request option changes (port, host, hostname, url) no longer affect the requests.

What is the Expected behavior?

  1. The hook intercepts WebSocket handshakes.
  2. Request option changes (port, host, hostname, url) affect the requests.

Same as without native automation, I expect for the hook to intercept all HTTP events, including fetch and WebSocket handshakes, and successfully send requests to the web service instance assigned to the test instance.

What is your TestCafe test code?

  async onRequest(e: any) {
    e.requestOptions.hostname = 'localhost';
    e.requestOptions.host = `localhost:${this.port}`;
    e.requestOptions.port = this.port.toString();
  }

Steps to Reproduce

  1. Add RequestHook that rewrites the request destination, for fetch requests and WebSocket handshakes.
  2. Check that it works without native automation.
  3. Check that it doesn't with native automation.

TestCafe version

2.5.0

Node.js version

18.15.0

Command-line arguments

testcafe chrome fixture.ts --test="Test anme" --live --native-automation

Browser name(s) and version(s)

Chrome 112

Platform(s) and version(s)

Windows 10 19045.2846

@Klaster1 Klaster1 added the TYPE: bug The described behavior is considered as wrong (bug). label Apr 19, 2023
@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label Apr 19, 2023
@miherlosev
Copy link
Collaborator

Hi @Klaster1

The hook does not intercept WebSocket handshakes.

Could you please describe why you need intercepting the WebSocket handshakes?

@need-response-app need-response-app bot removed the STATE: Need response An issue that requires a response or attention from the team. label Apr 20, 2023
@miherlosev miherlosev added the STATE: Need clarification An issue lacks information for further research. label Apr 20, 2023
@Klaster1
Copy link
Author

Klaster1 commented Apr 20, 2023

The app under test works with HTTP and WebSocket endpoints, I have to mock data from all sources. Mocking only HTTP requests would results in inadequate coverage. During tests, instead of using RequestMock, I start a lightweight express mock app per test and redirect all API calls to it. The same express app is used during development. This approach proved much more flexible than basic RequestMock.

@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label Apr 20, 2023
@github-actions github-actions bot removed the STATE: Need clarification An issue lacks information for further research. label Apr 20, 2023
@AlexKamaev
Copy link
Contributor

@Klaster1, I was able to reproduce the RequestHook event modification issue. Let's focus on this issue in this thread for now.

We still do not clearly understand why you need to intercept WebSocket handshakes. Why do you need some code coverage for handshakes? Please share more details with us and create a separate issue for this purpose.

@need-response-app need-response-app bot removed the STATE: Need response An issue that requires a response or attention from the team. label Apr 21, 2023
@Klaster1
Copy link
Author

Klaster1 commented Apr 21, 2023

@AlexKamaev sure, here are more details about my setup.

Parts involved:

  1. A single page application (SPA) that communicates with the backend (BE) APIs over HTTP and web sockets.
  2. A BE that exposes APIs over HTTP and web sockets. Big, unwieldy, time consuming to develop the FE against.
  3. A lightweight, purpose-built, Node.js-based (an express app) fake BE with roughly the same API surface as the real thing. Let's call it the "tester". Provides an ergonomic way to manipulate test data.

As a FE-developer, I want to cover the FE with tests, which I use TestCafe for. I do not want to perform complete integration testing of FE+BE, so I create an instance of a tester for each test. The same thing's used for tests and development.

Here's an excerpt of the integration layer that I use to instantiate a tester for each test:

// tester-integration.ts
export class GmcTesterHook extends RequestHook {
  constructor(private port: number) {
    super([/\/browser/, /\/api/]);
    this.tester = new GmcTester(this.port)
  }
  private tester: GmcTester;
  static async init(): Promise<GmcTesterHook> {
    const port = await getPort({ port: getPort.makeRange(3001, 3200) });
    const hook = new GmcTesterHook(port);
    await hook.tester.ready;
    return hook;
  }
  async onRequest(e: any) {
    e.requestOptions.hostname = 'localhost';
    e.requestOptions.host = `localhost:${this.port}`;
    e.requestOptions.port = this.port.toString();
  }
  async onResponse() {}
  async destroy() {
    await this.tester.destroy();
  }
  use(fn: HookFn) {
    this.tester.use((t) => fn(t, this.store));
    return this;
  }
}

export interface GmcTestController<T extends any = {}> extends TestController {
  ctx: T & { hook: GmcTesterHook };
}

// test.ts
test.before(async (t: GmcTestController<{hook: GmcTester}>) => {
  t.ctx.hook = await GmcTesterHook.init();
  t.updateState((draft) => {
    draft.users.add(makeUser())
  })
})

I hope this makes the need to mock web sockets clearer. Standard TestCafe APIs, such as RequestMock, only allow to mock HTTP responses. Without a way to mock web socket APIs (see #3832), the total app surface that can be tested reduces dramatically - this is what I called "inadequate coverage", as in "maximum test coverage of the app under test". Basically, I use a RequestHook as a proxy to redirect API requests to the matching tester instance.

@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label Apr 21, 2023
@AlexKamaev
Copy link
Contributor

@Klaster1,

Thank you for sharing your usage scenario. Please create a separate issue about WebSockets in the Native Automation mode. Let's keep this thread only for the Request Hook redirect issue.

@need-response-app need-response-app bot removed the STATE: Need response An issue that requires a response or attention from the team. label Apr 26, 2023
@Klaster1
Copy link
Author

Klaster1 commented May 5, 2023

According to the changelog, the 2.6.0-rc1 addresses my problem, so I updated to it and run the suite again. The mocked requests are now getting blocked by something, resulting in net::ERR_BLOCKED_BY_CLIENT run-time error and blocked:other in Chrome request inspector, disabling Windows built-in firewall and antivirus did not help. Same result on Windows 11 and 10.

Another thing I noticed is that my other hook I use to mock Stripe calls doesn't work under NA too:

RequestMock()
  .onRequestTo({ url: /setup_intents/ })
  .respond((req, res) => {
    res.statusCode = 200;
    res.headers = { 'Access-Control-Allow-Origin': '*' };
    res.setBody(
      JSON.stringify({
        payment_method: 'pm_12345',
      }),
    );
  });

@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label May 5, 2023
@AlexKamaev
Copy link
Contributor

AlexKamaev commented May 8, 2023

@Klaster1
We fixed the issue based on your description:

async onRequest(e: any) {
    e.requestOptions.hostname = 'localhost';
    e.requestOptions.host = `localhost:${this.port}`;
    e.requestOptions.port = this.port.toString();
}

The test code is here:
https://github.com/DevExpress/testcafe/blob/master/test/functional/fixtures/regression/gh-7640/testcafe-fixtures/index.js
Please try running the test code on your machine.
If the test passes but your usage scenario is still not working, please modify our test or share your example.

@need-response-app need-response-app bot removed the STATE: Need response An issue that requires a response or attention from the team. label May 8, 2023
@AlexKamaev AlexKamaev added the STATE: Need clarification An issue lacks information for further research. label May 8, 2023
@Klaster1
Copy link
Author

Klaster1 commented May 8, 2023

@AlexKamaev here's a test that reproduces the Stripe hook regression. Works without native automation, and fails with.

import { RequestMock, Selector } from 'testcafe';

fixture('Debug');

test('Debug', async (t) => {
  const hook = RequestMock()
    .onRequestTo({ url: /setup_intents/ })
    .respond((req, res) => {
      res.statusCode = 500;
      res.headers = { 'Access-Control-Allow-Origin': '*' };
      res.setBody(JSON.stringify({ error: 'Aborted by hook' }));
    });
  await t.addRequestHooks(hook);

  await t.navigateTo('https://portal.gridgain.com');
  await t.typeText(Selector('[gta="email_input"]'), 'iborisov+7640@gridgain.com');
  await t.typeText(Selector('[gta="pwd_input"]'), '1');
  await t.click(Selector('[gta="sign_in_btn"]'));
  await t.expect(Selector('[gta="create-nebula-cluster-button"]').hasAttribute('disabled')).notOk();
  await t.click(Selector('[gta="create-nebula-cluster-button"]'));
  await t.expect(Selector('dialog-provision-cluster').exists).ok();
  await t.click(Selector('.price__link'));
  await t
    .typeText(Selector(`[gta='dialog-update-credit-card__cardholder-name-input']`), 'Mighty Pirate')
    .switchToIframe(Selector(`[gta='dialog-update-credit-card__card-number-input'] iframe`))
    .typeText(Selector('input.Input'), '4242424242424242')
    .switchToMainWindow()
    .pressKey('tab')
    .switchToIframe(Selector(`[gta='dialog-update-credit-card__card-expiry-input'] iframe`))
    .typeText(Selector('input.Input'), '1030')
    .switchToMainWindow()
    .pressKey('tab')
    .switchToIframe(Selector(`[gta='dialog-update-credit-card__card-cvc-input'] iframe`))
    .typeText(Selector('input.Input'), '666')
    .switchToMainWindow()
    .pressKey('tab')
    .typeText(Selector(`[gta='dialog-update-credit-card__street-address-input']`), 'Scumm bar')
    .typeText(Selector(`[gta='dialog-update-credit-card__city-input']`), 'Melee Town')
    .typeText(Selector(`[gta='dialog-update-credit-card__region-input']`), 'Melee Island')
    .typeText(Selector(`[gta='dialog-update-credit-card__zip-input']`), '191040')
    .click(Selector(`[gta='dialog-update-credit-card__country-input']`))
    .click(Selector('mat-option').withText('Jamaica'))
    .typeText(Selector(`[gta='dialog-update-credit-card__company-input']`), 'The Pirate Company')
    .click(Selector(`[gta='dialog-update-credit-card__terms-input']`))
    .click(Selector(`[gta='dialog-update-credit-card__submit-button']`));
  await t.expect(Selector('.snackbar-error').textContent).contains('An unexpected error occurred');
});

As to the net::ERR_BLOCKED_BY_CLIENT, I was not yet able to determine if the cause lies in TestCafe or my convoluted proxy setup. Will investigate further later, at least I know a workaround for this, unlike the Stripe hook regression.

@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label May 8, 2023
@github-actions github-actions bot removed the STATE: Need clarification An issue lacks information for further research. label May 8, 2023
@AlexKamaev
Copy link
Contributor

@Klaster1 Thank you for sharing your code. I have managed to reproduce the issue. It looks like for some reason the mock code is not called in Native Automation mode.

@need-response-app need-response-app bot removed the STATE: Need response An issue that requires a response or attention from the team. label May 10, 2023
@AlexKamaev AlexKamaev removed their assignment May 10, 2023
@miherlosev miherlosev self-assigned this May 11, 2023
@miherlosev
Copy link
Collaborator

The example from #7640 (comment) works correctly.
If you perform the test steps manually, you will get another error message:

Also, it means that in regular mode, request processing works incorrectly for this use case.
In my opinion, the best solution here is to make two expected error messages for native automation and regular modes and use the necessary one depending on the mode in use.

@Klaster1
Copy link
Author

Klaster1 commented May 12, 2023

@miherlosev I'm not sure what you mean. Did you manage to address the discrepancy between the regular and native automation modes in the Stripe request hook? To clarify, the request hook was supposed to intercept Stripe requests, preventing them from hitting the matching endpoints. That doesn't work in native automation.

@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label May 12, 2023
@AlexKamaev
Copy link
Contributor

I'm still able to reproduce the described issue. I'll reopen it.

@AlexKamaev AlexKamaev reopened this May 15, 2023
@need-response-app need-response-app bot removed the STATE: Need response An issue that requires a response or attention from the team. label May 15, 2023
@miherlosev miherlosev assigned AlexKamaev and unassigned miherlosev May 16, 2023
@AlexKamaev
Copy link
Contributor

@Klaster1
I researched the issue with request mock of the setup_intents request. It seems that I found the cause. However, your website is unavailable now for some reason, so I cannot check if my fix works. Please check your usage scenario with the following testcafe package which contains the fix: https://github.com/AlexKamaev/issues/raw/master/testcafe-2.6.1.tgz
Please share you results with us.

@AlexKamaev AlexKamaev added the STATE: Need clarification An issue lacks information for further research. label May 19, 2023
@Klaster1
Copy link
Author

Klaster1 commented May 19, 2023

@AlexKamaev I've created a new test account, please update the fixture to use "iborisov+7640+2@gridgain.com" instead of "iborisov+7640@gridgain.com". If the same happens again, feel free to create another account.

With the new build, Stripe iframes do not load at all with the NA enabled. The regular mode still works as expected. I can't tell if the fix helps because of a new regression.

image

 Debug
 × Debug (screenshots: C:\dev\gmc\modules\frontend\e2e\screenshots\2023-05-19_14-38-38\test-1\Chrome_113.0.0.0_Windows_11\errors\1.png)

   1) Content of the iframe to which you are switching did not load.

      Browser: Chrome 113.0.0.0 / Windows 11
      Screenshot: C:\dev\gmc\modules\frontend\e2e\screenshots\2023-05-19_14-38-38\test-1\Chrome_113.0.0.0_Windows_11\errors\1.png

         20 |  await t.click(Selector('[gta="create-nebula-cluster-button"]'));
         21 |  await t.expect(Selector('dialog-provision-cluster').exists).ok();
         22 |  await t.click(Selector('.price__link'));
         23 |  await t
         24 |    .typeText(Selector(`[gta='dialog-update-credit-card__cardholder-name-input']`), 'Mighty Pirate')
       > 25 |    .switchToIframe(Selector(`[gta='dialog-update-credit-card__card-number-input'] iframe`))
         26 |    .typeText(Selector('input.Input'), '4242424242424242')
         27 |    .switchToMainWindow()
         28 |    .pressKey('tab')
         29 |    .switchToIframe(Selector(`[gta='dialog-update-credit-card__card-expiry-input'] iframe`))
         30 |    .typeText(Selector('input.Input'), '1030')

         at <anonymous> (C:\dev\gmc\modules\frontend\e2e\fixtures-hosted\debug.ts:25:6)
         at fulfilled (C:\dev\gmc\modules\frontend\e2e\fixtures-hosted\debug.ts:5:58)

@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label May 19, 2023
@github-actions github-actions bot removed the STATE: Need clarification An issue lacks information for further research. label May 19, 2023
@AlexKamaev
Copy link
Contributor

@Klaster1,
I will check my fix one more time. Thank you for your cooperation.

@need-response-app need-response-app bot removed the STATE: Need response An issue that requires a response or attention from the team. label May 19, 2023
miherlosev pushed a commit that referenced this issue May 23, 2023
this should fix #7640 issue with request interception

- implement the `attachedToTarget` event for iframes
- call Fetch.enable for iframes
- add the `Runtime.runIfWaitingForDebugger` call just to make this thing
work (playwright does the same trick).
- ignore typing errors for sessionId argument
(cyrus-and/chrome-remote-interface#534)
- I did not manage to create a test due to its complexity and unknown
cause
@github-actions
Copy link

Release v2.6.1-rc.1 addresses this.

Aleksey28 pushed a commit that referenced this issue Jun 13, 2023
Tests fail now, because the Native Automation task is in non-graphic
mode.
Tests should pass after merge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SYSTEM: native automation TYPE: bug The described behavior is considered as wrong (bug).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants