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

fix: no-sw error shows for subdomain requests #491

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -226,3 +226,4 @@ playwright-report
.coverage
test-e2e/fixtures/data/test-repo
test-e2e/fixtures/data/gateway-conformance-fixtures
test-results
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
"test:iso": "aegir test -f dist-tsc/test/**/*.spec.js",
"test:browsers": "playwright test -c playwright.config.js",
"test:chrome": "playwright test -c playwright.config.js --project chromium",
"test:firefox": "playwright test -c playwright.config.js --project firefox",
"test:no-sw": "playwright test -c playwright.config.js --project no-service-worker",
"test:firefox": "playwright test -c playwright.config.js --project firefox --project no-service-worker",
"test:deployed": "playwright test -c playwright.config.js --project deployed",
"test:inbrowser-dev": "cross-env BASE_URL='https://inbrowser.dev' playwright test -c playwright.config.js --project deployed",
"test:inbrowser-prod": "cross-env BASE_URL='https://inbrowser.link' playwright test -c playwright.config.js --project deployed",
Expand Down
26 changes: 26 additions & 0 deletions playwright.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,32 @@ export default defineConfig({
...devices['Desktop Firefox'],
baseURL: process.env.BASE_URL
}
},
{
/**
* Test the site with service workers disabled. You need to `import {testNoServiceWorker as test, expect} from './fixtures/config-test-fixtures.js'` to use this project.
* Anything needing a service worker will be skipped when this project is ran.
*/
name: 'no-service-worker',
testMatch: /test-e2e\/no-service-worker\.test\.ts/,
use: {
...devices['Desktop Firefox'],
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this doesn't test no service worker functionality in chrome, but we shouldn't need to...?

contextOptions: {
serviceWorkers: 'block'
},
launchOptions: {
firefoxUserPrefs: {
'dom.serviceWorkers.enabled': false
}
},
beforeEach: async ({ page }) => {
await page.addInitScript(() => {
Object.defineProperty(navigator, 'serviceWorker', {
get: () => undefined
})
})
}
}
}
],
// TODO: disable webservers when testing `deployed` project
Expand Down
1 change: 1 addition & 0 deletions src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ if ('serviceWorker' in navigator) {

const routes: Route[] = [
{ default: true, component: ErrorPage ?? LazyHelperUi },
{ shouldRender: async () => renderChecks.shouldRenderNoServiceWorkerError(), component: LazyServiceWorkerErrorPage },
{ shouldRender: async () => renderChecks.shouldRenderRedirectsInterstitial(), component: LazyInterstitial },
Comment on lines 24 to 26
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 is the actual bug fix..

{ path: '#/ipfs-sw-config', shouldRender: async () => renderChecks.shouldRenderConfigPage(), component: LazyConfig },
{
Expand Down
4 changes: 4 additions & 0 deletions src/lib/routing-render-checks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,7 @@ export function shouldRenderRedirectsInterstitial (): boolean {
const heliaSw = url.searchParams.get('helia-sw')
return heliaSw != null
}

export function shouldRenderNoServiceWorkerError (): boolean {
return !('serviceWorker' in navigator)
}
2 changes: 1 addition & 1 deletion src/pages/errors/no-service-worker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export default function NoServiceWorkerErrorPage (): React.JSX.Element {
return (
<>
<Header />
<main className='pa4-l bg-red-muted mw7 mb5 center pa4'>
<main className='pa4-l bg-red-muted mw7 mb5 center pa4 e2e-no-service-worker-error'>
<h1>Service Worker Error</h1>
<p>
This page requires a service worker to be available. Please ensure that
Expand Down
34 changes: 31 additions & 3 deletions test-e2e/fixtures/config-test-fixtures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ import { test as base, type Page } from '@playwright/test'
import { setConfig, setSubdomainConfig } from './set-sw-config.js'
import { waitForServiceWorker } from './wait-for-service-worker.js'

function isNoServiceWorkerProject <T extends typeof base = typeof base> (test: T): boolean {
return test.info().project.name === 'no-service-worker'
}

const rootDomain = async ({ baseURL }, use): Promise<void> => {
const url = new URL(baseURL)
await use(url.host)
Expand All @@ -13,13 +17,20 @@ const baseURLProtocol = async ({ baseURL }, use): Promise<void> => {

export const test = base.extend<{ rootDomain: string, baseURL: string, protocol: string }>({
rootDomain: [rootDomain, { scope: 'test' }],
protocol: [baseURLProtocol, { scope: 'test' }]
protocol: [baseURLProtocol, { scope: 'test' }],
page: async ({ page }, use) => {
if (isNoServiceWorkerProject(test)) {
test.skip()
return
}
await use(page)
}
})

/**
* You should use this fixture instead of the `test` fixture from `@playwright/test` when testing path routing via the service worker.
*/
export const testPathRouting = base.extend<{ rootDomain: string, baseURL: string, protocol: string }>({
export const testPathRouting = test.extend<{ rootDomain: string, baseURL: string, protocol: string }>({
Copy link
Member Author

Choose a reason for hiding this comment

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

idk why this wasn't being done before, but all of our custom test fixtures now extend from our test fixture above, so we get the isNoServiceWorkerProject check for free without modifying all the others.

rootDomain: [rootDomain, { scope: 'test' }],
protocol: [baseURLProtocol, { scope: 'test' }],
page: async ({ page, rootDomain }, use) => {
Expand Down Expand Up @@ -63,7 +74,7 @@ export const testPathRouting = base.extend<{ rootDomain: string, baseURL: string
* })
* ```
*/
export const testSubdomainRouting = base.extend<{ rootDomain: string, baseURL: string, protocol: string }>({
export const testSubdomainRouting = test.extend<{ rootDomain: string, baseURL: string, protocol: string }>({
rootDomain: [rootDomain, { scope: 'test' }],
protocol: [baseURLProtocol, { scope: 'test' }],
page: async ({ page, baseURL }, use) => {
Expand Down Expand Up @@ -108,4 +119,21 @@ export const testSubdomainRouting = base.extend<{ rootDomain: string, baseURL: s
}
})

/**
* A fixture that skips tests that require the service worker. This is needed in order to test handling of requests where the service worker is not present.
*
* @see https://github.com/ipfs/service-worker-gateway/issues/272
*/
export const testNoServiceWorker = base.extend<{ rootDomain: string, baseURL: string, protocol: string }>({
Copy link
Member Author

Choose a reason for hiding this comment

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

not extending our test fixture above because that is trying to skip everything that skips tests on the isNoServiceWorkerProject(test) check. we need the opposite here.

rootDomain: [rootDomain, { scope: 'test' }],
protocol: [baseURLProtocol, { scope: 'test' }],
page: async ({ page }, use) => {
if (!isNoServiceWorkerProject(testNoServiceWorker)) {
testNoServiceWorker.skip()
return
}
await use(page)
}
})

export { expect } from '@playwright/test'
2 changes: 2 additions & 0 deletions test-e2e/fixtures/locators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ export const getConfigGatewaysInput: GetLocator = (page) => page.locator('.e2e-c
export const getConfigRoutersInput: GetLocator = (page) => page.locator('.e2e-config-page-input-routers')
export const getConfigAutoReloadInput: GetLocator = (page) => page.locator('.e2e-config-page-input-autoreload')

export const getNoServiceWorkerError: GetLocator = (page) => page.locator('.e2e-no-service-worker-error')

/**
* Iframe page parts
*/
Expand Down
2 changes: 1 addition & 1 deletion test-e2e/layout.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { test, expect } from '@playwright/test'
import { test, expect } from './fixtures/config-test-fixtures.js'
import { getConfigButton, getConfigPage, getConfigPageSaveButton, getConfigPageInput, getHeader, getHeaderTitle } from './fixtures/locators.js'

test.describe('smoketests', () => {
Expand Down
16 changes: 16 additions & 0 deletions test-e2e/no-service-worker.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { testNoServiceWorker as test, expect } from './fixtures/config-test-fixtures.js'
import { getNoServiceWorkerError } from './fixtures/locators.js'

test.describe('no-service-worker', () => {
test('Error renders on landing page', async ({ page }) => {
await page.goto('/')

await expect(getNoServiceWorkerError(page)).toBeVisible()
})

test('Error renders on subdomain page', async ({ page, rootDomain, protocol }) => {
await page.goto(`${protocol ?? 'http'}//bafkqablimvwgy3y.ipfs.${rootDomain}`, { waitUntil: 'networkidle' })

await expect(getNoServiceWorkerError(page)).toBeVisible()
})
})
Loading