Skip to content

Conversation

LeoMcA
Copy link
Member

@LeoMcA LeoMcA commented Oct 17, 2025

There's some slightly complicated caching logic to port to fastly, manually testing seems like a bad idea.

Draft while this is a work in progress.

Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

Overall looking good.

Nits:

  • Default to sending accept-language: en on all requests, as that would be more realistic.
  • Extract some urls as constants.
  • Extract a helper function that avoids duplication and makes the test cases more readable (three parameters: original URL, expected URL, options { acceptLanguage, preferredLanguage })

PS: Note that conventional commits includes a test: prefix.

Comment on lines +8 to +9
let response = await fetch(new URL("/en-US/docs/Web", BASE_URL));
assert.ok(response.url.endsWith("/en-US/docs/Web"), response.url);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to skip the test instead (if that URL is not available)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Before landing this test, we should make sure that running the tests locally doesn't make requests to stage.

Afaik, if we just define the test like this, then node --test . would execute it.

cookie: "preferredlocale=fr",
}),
});
assert.ok(response.url.endsWith("/fr/docs/Web"), response.url);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert.ok(response.url.endsWith("/fr/docs/Web"), response.url);
assert.strictEqual("/fr/docs/Web", URL.parse(response.url).pathname);

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