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

test: add tests for page level redirects #45

Merged
merged 8 commits into from
Aug 20, 2024
Merged

test: add tests for page level redirects #45

merged 8 commits into from
Aug 20, 2024

Conversation

FentPams
Copy link
Contributor

@FentPams FentPams commented Aug 19, 2024

Description

To test:

  1. RUM is fired before redirect
  2. Page is successfully redirected in all three cases (experiments, campaigns, audiences)

Finding:

  1. Tried different ways to override the window.location.replace () to let it do nothing when testing RUM is fired before redirect.
    However, some browsers have strict security restriction that don't allow certain function to be overridden, which leads to test fails.
  2. Modify the script before its execution. The line with window.location.replace () is commented out before the script is executed and security policies are applied.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@FentPams FentPams marked this pull request as ready for review August 19, 2024 15:19
@FentPams FentPams requested a review from ramboz August 19, 2024 15:19
Copy link

codecov bot commented Aug 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (v2@285011f). Learn more about missing BASE report.

Additional details and impacted files
@@          Coverage Diff          @@
##             v2      #45   +/-   ##
=====================================
  Coverage      ?   93.60%           
=====================================
  Files         ?        1           
  Lines         ?      704           
  Branches      ?       11           
=====================================
  Hits          ?      659           
  Misses        ?       34           
  Partials      ?       11           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tests/audiences.test.js Outdated Show resolved Hide resolved
@ramboz ramboz changed the title Add tests for page level redirect test: add tests for page level redirects Aug 20, 2024
@FentPams
Copy link
Contributor Author

@ramboz
I was inspired by what you said, the test fail because the control page will not trigger replace(), which waitForUrl will keep loading and then timeout.

Then I thought of leveraging page.waitForFunction https://playwright.dev/docs/api/class-frame#frame-wait-for-function , because goto ('/tests/fixtures/experiments/page-level--redirect' )do have chance to land on the control page. By using the await page.waitForFunction(() => window.hlx.rum.sampleRUM);, we can test closer to the real case without forcing a variant (the code below).

Then this sparks my thinking that "Should we allow serving the control page if the property Experiment Resolution : redirect is defined in page metadata? Even though this behavior will not trigger the redirection, the browser still gets chance to land on the control page in our current logic. If we want to ensure to only serve variants instead and redirect can happen once they have redirect defined, then we need to modify our logic in getExperimentConfig (e.g. if keyword redirect exists, then config.selectedVaraint will choose from the variant pages other than control page....). Do you think this change could be necessary?

I really appreciate if you could provide any thoughts here :)

test('Track RUM is fired before redirect 2 ', async ({ page }) => {
    const rumCalls = [];
    await page.exposeFunction('logRumCall', (...args) => rumCalls.push(args));
    await page.addInitScript(() => {
      window.hlx = { rum: { sampleRUM: (...args) => window.logRumCall(args) } };
    });
    
    await page.goto('/tests/fixtures/experiments/page-level--redirect');

    // verify rum is fired 
    await page.waitForFunction(() => window.hlx.rum.sampleRUM);
    expect(rumCalls[0]).toContainEqual([
      'experiment',
      {
        source: 'foo',
        target: expect.stringMatching(/control|challenger-1|challenger-2/),
      },
    ]);

    // verify page is redirected
    const expectedContent = {
      'v1': 'Hello v1!',
      'v2': 'Hello v2!',
      'redirect': 'Hello World!'
    };

    const expectedUrlPath = {
      'v1': '/tests/fixtures/experiments/page-level-v1',
      'v2': '/tests/fixtures/experiments/page-level-v2',
      'redirect': '/tests/fixtures/experiments/page-level--redirect'
    };

    const url = new URL(page.url());
    const variant = Object.keys(expectedUrlPath).find(k => url.pathname.endsWith(k));
    expect(await page.evaluate(() => window.document.body.innerText)).toMatch(new RegExp(expectedContent[variant]));
    expect(expectedUrlPath[variant]).toBe(url.pathname);
  });

@ramboz
Copy link
Contributor

ramboz commented Aug 20, 2024

@FentPams Good idea with the test. I think this is definitely valid as well.
As to:

Should we allow serving the control page if the property Experiment Resolution : redirect is defined in page metadata?

Yes, we should. if the customer wants to force the redirect and not use the control, then they can set the split ratio to 0 for the control

@FentPams FentPams merged commit d4a5c00 into v2 Aug 20, 2024
5 checks passed
@FentPams FentPams deleted the redirect-test branch August 20, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants