Add testing to the "don’t generate invalid <link rel="canonical"/> with missing href" solution

[erbierc]

Description

Continuation of #3420 and #3496
I have modified the getRouteDataTestContext() function to make sure I can get test context without site set, same with getTestHead(). The new parameter is optional, so it does not break any other usage of the functions.
I also wrote a test checking that <meta property="og:url"/> with missing content is not generated. The test passes.

I wanted to write one more test checking <link rel="canonical"/> in the same way (if site is not set, this element should not appear). However, getRouteDataTestContext() has a return type of

type RouteDataContext = {
    generator: string;
    site: URL | undefined;
    url: AstroSharedContext["url"];
}

or, more specifically:

type RouteDataContext = Pick<APIContext, 'generator' | 'site' | 'url'>;

url is required, and that url actually sets <link rel="canonical"/> to it, from what I logged in the console.

I would kindly ask you to check if my test logic is alright, as I have not done a lot of testing before and would appreciate any help with figuring out what to do to be able to write the next test. I don't really want to mess with existing types and I don't know the codebase well enough yet to come up with something new.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 0a8a1bd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[delucis]

url is required, and that url actually sets <link rel="canonical"/> to it, from what I logged in the console.

Ah, that could suggest the logic in utils/head.ts needs further changes potentially! The type there is accurate because it reflects the value Astro provides, and it always provides a value for url even when site is not set:

When building a site, prerendered routes will receive a URL based on the site and base options. If site is not configured, prerendered pages will receive a localhost URL during builds as well. (Source: https://docs.astro.build/en/reference/api-reference/#url)

So it’s correct that the getRouteDataTestContext() always returns url, and if that means we don’t get the omission we’re looking for, we’ll need to do a bit more work in the head logic I guess. So writing a failing test for the canonical is a good start.

The changes to getRouteDataTestContext() look good 👍 I think we might eventually want to refactor it to use an options object to avoid the slightly awkward cases like getRouteDataTestContext(undefined, false) which would be clearer as getRouteDataTestContext({ setSite: false }). But we can do that in a follow-up PR where we’re not changing any other logic or tests.

---

P.S. I just updated this branch so it has everything from the target branch — you might want to make sure you pull before making your next changes.

[erbierc]

Ok, I had some git confusion there. Added the failing test!

[erbierc]

Btw I just wanted to add that I tested this manually by packing my local astro version (with the canonical changes) and comparing it to the current release, the invalid canonical and og:url did, in fact, disappear

[erbierc]

@delucis Could you please take a look at vitest.config.ts for head tests? I think this is the reason why the test fails. We keep getting

{
    tag: 'link',
    attrs: { rel: 'canonical', href: 'https://example.com/test' }
  },

in the head, no matter the test setup, because config has it by default.

I noticed that because getRouteDataTestContext() sets the URL to https://example.com, not https://example.com/test, and yet the test part kept appearing in logs. That might be it! :D

[delucis]

@delucis Could you please take a look at vitest.config.ts for head tests? I think this is the reason why the test fails

Ah yes, that makes sense! We were already testing that canonicals get deduplicated, so we set that in the test config to make sure the user version is used.

I guess in this case we can instead add the new test to our “Basics” test folder. Those tests run with a minimal config, so should work in this case, I think?

Thanks for figuring that out!