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: e2e subdomain redirect tests #151

Merged
merged 7 commits into from
Mar 25, 2024
Merged

Conversation

SgtPooki
Copy link
Member

@SgtPooki SgtPooki commented Mar 23, 2024

  • test: add reverse-proxy for e2e tests
  • test: subdomain-detection test
  • fix: do not render redirectPage if not parent page
  • test: subdomain-detection with autoreload test

Title

test: e2e subdomain redirect tests

Description

subdomain redirect tests related to #134

Summary of changes

  1. add reverse-proxy for e2e tests
  2. add subdomain-detection test
  3. do not render redirectPage if not parent page
  4. add subdomain-detection with autoreload test
  5. move locators for e2e tests to a separate file

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (89b3ace) to head (857f0e8).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #151   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           35        35           
  Branches         5         5           
=========================================
  Hits            35        35           
Flag Coverage Δ
node 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Member Author

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

self review

serviceWorkers: 'allow'
serviceWorkers: 'allow',

ignoreHTTPSErrors: true
Copy link
Member Author

Choose a reason for hiding this comment

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

we don't actually need this anymore

Suggested change
ignoreHTTPSErrors: true

src/sw.ts Outdated Show resolved Hide resolved
@@ -460,7 +465,7 @@ async function fetchHandler ({ path, request, event }: FetchHandlerArg): Promise
const response = await verifiedFetch(verifiedFetchUrl, {
signal,
headers,
// TODO redirect: 'manual', // enable when http urls are supported by verified-fetch: https://github.com/ipfs-shipyard/helia-service-worker-gateway/issues/62#issuecomment-1977661456
redirect: 'manual',
Copy link
Member Author

Choose a reason for hiding this comment

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

finally done with this todo

Copy link
Member

Choose a reason for hiding this comment

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

That would close #62

Copy link
Member Author

Choose a reason for hiding this comment

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

i'll add an e2e test prior to closing #62

Copy link
Member Author

Choose a reason for hiding this comment

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

one last thing blocking fix of #62: ipfs/helia-verified-fetch#33

it was previously only working for subpath directories.

channel.postMessage({ target: 'SW', action: 'RELOAD_CONFIG', source: 'WINDOW' })
await swResponsePromise
// TODO: we shouldn't need this. We should be able to just post a message to the service worker to reload it's config.
window.postMessage({ source: 'helia-sw-config-iframe', target: 'PARENT', action: 'RELOAD_CONFIG', config: configInPage }, { targetOrigin: window.location.origin })
Copy link
Member Author

@SgtPooki SgtPooki Mar 23, 2024

Choose a reason for hiding this comment

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

this is actually the only part that works triggers the config reload on subdomains.. without this, it doesn't work in the autoreload test, however, the above is needed to set the config on origin domain.

I confirmed that origin is getting indexedDB set properly, but autoreload doesn't get picked up by subdomain for some reason.

test-e2e/path-routing.test.ts Show resolved Hide resolved
test-e2e/path-routing.test.ts Show resolved Hide resolved
@@ -0,0 +1,33 @@
/**
* This is a simple reverse proxy that makes sure that any request to localhost:3333 is forwarded to localhost:3000.
Copy link
Member Author

Choose a reason for hiding this comment

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

this should also make it easier for devs to contribute without having to install traefik/nginx/caddy or do anything fancy

expect(initialResponse?.request()?.redirectedFrom()?.url()).toBe('http://localhost:3333/ipfs/bafkqablimvwgy3y')

await page.waitForURL('http://bafkqablimvwgy3y.ipfs.localhost:3333')
await setConfig({ page, config: { autoReload: true } })
Copy link
Member Author

Choose a reason for hiding this comment

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

ideally, setConfig would occur immediately after line 30, but.. alas, subdomains in playwright are not picking up the config for some reason.

Comment on lines +49 to +59
const channel = new BroadcastChannel('helia:sw')
const swResponsePromise = new Promise<void>((resolve, reject) => {
const onSuccess = (e: MessageEvent): void => {
if (e.data.action === 'RELOAD_CONFIG_SUCCESS') {
resolve()
channel.removeEventListener('message', onSuccess)
}
}
channel.addEventListener('message', onSuccess)
})
channel.postMessage({ target: 'SW', action: 'RELOAD_CONFIG', source: 'WINDOW' })
Copy link
Member Author

Choose a reason for hiding this comment

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

I hope this code here helps people understand the benefit of HeliaServiceWorkerCommsChannel, even if it does need cleaned up further.

@SgtPooki SgtPooki requested a review from lidel March 23, 2024 05:06
@SgtPooki SgtPooki self-assigned this Mar 23, 2024
@SgtPooki SgtPooki requested a review from 2color March 23, 2024 05:06
@SgtPooki SgtPooki mentioned this pull request Mar 23, 2024
14 tasks
}
webServer: [
{
command: 'node test-e2e/reverse-proxy.js',
Copy link
Member

Choose a reason for hiding this comment

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

Why is the reverse proxy needed? Currently it only seems to add CORS headers and allow OPTIONS requests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

╰─ ✔ ❯ npm run test:chrome -- -g 'subdomain-detection' --max-failures 5

> helia-service-worker-gateway@1.0.0 test:chrome
> playwright test -c playwright.config.js --project chromium -g subdomain-detection --max-failures 5

[WebServer] (node:14358) [DEP0066] DeprecationWarning: OutgoingMessage.prototype._headers is deprecated
(Use `node --trace-deprecation ...` to show where the warning was created)

Running 2 tests using 1 worker

  ✘  1 [chromium] › subdomain-detection.test.ts:6:3 › subdomain-detection › path requests are redirected to subdomains (898ms)
  ✘  2 [chromium] › subdomain-detection.test.ts:28:3 › subdomain-detection › path requests are redirected to subdomains automatically with autoreload enabled (893ms)


  1) [chromium] › subdomain-detection.test.ts:6:3 › subdomain-detection › path requests are redirected to subdomains

    Error: expect(received).toBe(expected) // Object.is equality

    Expected: "http://bafkqablimvwgy3y.ipfs.localhost:3333/"
    Received: "http://localhost:3000/ipfs/bafkqablimvwgy3y"

      11 |     const initialResponse = await page.goto('/ipfs/bafkqablimvwgy3y', { waitUntil: 'commit' })
      12 |
    > 13 |     expect(initialResponse?.url()).toBe('http://bafkqablimvwgy3y.ipfs.localhost:3333/')
         |                                    ^
      14 |     expect(initialResponse?.request()?.redirectedFrom()?.url()).toBe('http://localhost:3333/ipfs/bafkqablimvwgy3y')
      15 |
      16 |     await page.waitForURL('http://bafkqablimvwgy3y.ipfs.localhost:3333')

        at /Users/sgtpooki/code/work/protocol.ai/ipfs-shipyard/helia-service-worker-gateway/test-e2e/subdomain-detection.test.ts:13:36

  2) [chromium] › subdomain-detection.test.ts:28:3 › subdomain-detection › path requests are redirected to subdomains automatically with autoreload enabled

    Error: expect(received).toBe(expected) // Object.is equality

    Expected: "http://bafkqablimvwgy3y.ipfs.localhost:3333/"
    Received: "http://localhost:3000/ipfs/bafkqablimvwgy3y"

      32 |     const initialResponse = await page.goto('/ipfs/bafkqablimvwgy3y', { waitUntil: 'commit' })
      33 |
    > 34 |     expect(initialResponse?.url()).toBe('http://bafkqablimvwgy3y.ipfs.localhost:3333/')
         |                                    ^
      35 |     expect(initialResponse?.request()?.redirectedFrom()?.url()).toBe('http://localhost:3333/ipfs/bafkqablimvwgy3y')
      36 |
      37 |     await page.waitForURL('http://bafkqablimvwgy3y.ipfs.localhost:3333')

        at /Users/sgtpooki/code/work/protocol.ai/ipfs-shipyard/helia-service-worker-gateway/test-e2e/subdomain-detection.test.ts:34:36

  2 failed
    [chromium] › subdomain-detection.test.ts:6:3 › subdomain-detection › path requests are redirected to subdomains
    [chromium] › subdomain-detection.test.ts:28:3 › subdomain-detection › path requests are redirected to subdomains automatically with autoreload enabled

Copy link
Member Author

Choose a reason for hiding this comment

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

the ports are wrong, but you can see that it's not on a subdomain

@SgtPooki SgtPooki merged commit 2d146e2 into main Mar 25, 2024
22 checks passed
@SgtPooki SgtPooki deleted the test/e2e-subdomain-redirect branch March 25, 2024 22:26
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.

3 participants