Skip to content

refactor(test utils): Add options object to test utility functions (getRouteDataTestContext, getTestHead) #3544

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Nov 10, 2025
Merged

refactor(test utils): Add options object to test utility functions (getRouteDataTestContext, getTestHead) #3544

merged 3 commits into from
Nov 10, 2025

Conversation

@erbierc
Copy link
Contributor

@erbierc erbierc commented Nov 9, 2025

Created a new PR, as the previous one (#3542) was synced with main, thus making reviewing it a pain. And the one after that, #3543 was set to main, not stack/invalid-canonical 🤡

Description

  • Closes #N/A - follow up to Don’t generate invalid meta tags in <head> #3496
  • What does this PR change? Give us a brief description.
    In a previous PR, I have added tests to the invalid canonical changes. This required me to include additional arguments to getRouteDataTestContext and getTestHead functions. This refactor is meant to make these changes more readable and developer friendly by adding an options object to both of them. This PR also makes sure all usage of these functions are updated as well. As of now, all tests pass.
type GetTestHeadOptions = {
	heads?: HeadConfig,
	route?: Route | undefined,
	setSite?: boolean
}

Here, route has an additonal | undefined because of the 'includes description based on page description frontmatter field if provided' test in the same file (TypeScript doesn't like it without that).
Other options object type added:

type RouteDataTestContextOptions = {
	pathname?: string,
	setSite?: boolean
}

Note: I have placed the new types above these functions. If I need to move them, or if you would prefer interfaces instead, I'd really like to know!

@changeset-bot
Copy link

changeset-bot bot commented Nov 9, 2025

⚠️ No Changeset found

Latest commit: b4bee23

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

@github-actions github-actions bot added the 🌟 core Changes to Starlight’s main package label Nov 9, 2025
delucis
delucis approved these changes Nov 10, 2025
View reviewed changes
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Lovely work @erbierc 👏 Appreciate how you laid out the idea and the work in the description — really helpful.

Btw, re: the second false-start PR — you can actually switch the target after opening a PR using the Edit button in the top right. Clicking that will make the PR title editable, but also makes it possible to switch the branch target just below there.

Anyway, I think this means we can merge this and then get that fix merged too 🎉

@delucis delucis merged commit 36e651f into withastro:stack/invalid-canonical Nov 10, 2025
18 checks passed
@erbierc
Copy link
Contributor Author

erbierc commented Nov 10, 2025

@delucis Good to know, thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@delucis delucis delucis approved these changes

Assignees

No one assigned

Labels

🌟 core Changes to Starlight’s main package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@erbierc @delucis