Skip to content

Conversation

@JonathanMatthey
Copy link
Contributor

PR Checklist

  • Linked issue added (e.g., Fixes #123)
  • If styles were updated:
    • Stylesheet version has been bumped

Summary

Comment on lines +41 to +43
// If you later want the tests to start the local stack automatically,
// consider wiring a webServer here (e.g., make -C local up) and a custom
// timeout/wait condition. For now we expect the target site to be running.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// If you later want the tests to start the local stack automatically,
// consider wiring a webServer here (e.g., make -C local up) and a custom
// timeout/wait condition. For now we expect the target site to be running.

});

test('admin route is protected (403 or login redirect) for anonymous', async ({ page, context }) => {
const res = await context.request.get('/admin', { maxRedirects: 0 });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Surely this won't work since we use custom admin paths and the purpose of that is to not reveal them, so we should probably leave this out.

const isLoginRedirect = status >= 300 && status < 400 && /\/user\/login/i.test(location);
expect(status === 403 || isLoginRedirect).toBeTruthy();

await page.goto('/admin');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Surely this won't work since we use custom admin paths and the purpose of that is to not reveal them, so we should probably leave this out.

expect(canonical, 'canonical link should exist').toBeTruthy();

const robots = (await page.locator('meta[name="robots"]').getAttribute('content')) || '';
expect(/noindex/i.test(robots)).toBeFalsy();
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the README you mention testing against staging but this would fail then.

- Node.js 18+ (20+ recommended)
- Browsers for Playwright (`npm run install:browsers`)
- The target site running locally or accessible via URL

Copy link
Collaborator

Choose a reason for hiding this comment

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

Some of the local set up steps can also be called out as required more clearly, like having db and files in the correct local arrnagment

@JoblersTune
Copy link
Collaborator

When running the tests the screenshots and recordings are blank

image

@JoblersTune
Copy link
Collaborator

A bunch of tests are failing

image image image

@JoblersTune
Copy link
Collaborator

All of the Webkit tests fail for me. Do they work for you?

<head>
<head-placeholder token="{{ placeholder_token }}">
<title>{{ head_title|safe_join(' | ') }}</title>
<meta name="robots" content="index, follow">
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the purpose of adding this?

  1. It is the default behaviour, right?
  2. I'm just trying to be aware of how this might effect us blocking crawling and indexing of the staging site.

use: { ...devices['Desktop Firefox'] }
},
{
name: 'webkit',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Webkit just won't run correctly on my Linux machine. It is valuable to have here but when we push this to CI we might have issues there too, depending what environment we're running in.

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