Skip to content

Add trailing slash to manual tests link#2241

Merged
alexpetros merged 1 commit intobigskysoftware:devfrom
danielnieto:fix/trailing-slash-manual-tests-link
Jan 24, 2024
Merged

Add trailing slash to manual tests link#2241
alexpetros merged 1 commit intobigskysoftware:devfrom
danielnieto:fix/trailing-slash-manual-tests-link

Conversation

@danielnieto
Copy link
Copy Markdown
Contributor

@danielnieto danielnieto commented Jan 24, 2024

Description

When trying to contribute, docs suggest to use npx serve and then go to http://0.0.0.0:3000/test/ to see all tests, in this file there's a link to the manual tests, but if you have followed the steps on the guide you will realize that the links for the individual manual tests are not working, all files will throw a 404, the first time I saw this I didn't know if I had something misconfigured, then I realized that the links were relative and since the link to "manual" didn't have a trailing slash, all individual manual tests were pointed to the root of tests folder (i.e. http://localhost:3000/test/aborting-request instead of http://localhost:3000/test/manual/aborting-request)

The "correct" way to fix this would be to add a serve.json as stated here: vercel/serve#628, but not sure about adding a new config file and also, on my first test that approach worked for the test files links but broke relative image paths so I would need to review all tests and fix all affected. (not to mention that we need to make sure that paths works when open the files "locally" without npx serve)

Instead, I'm submitting this PR which only adds a trailing slash to the manual tests link on http://0.0.0.0:3000/test/ this way a newcomer would not get immediately confused when trying to run and explore tests.

I've made sure that this approach also works when opening the files "locally".

Screenshot of the issue:
image

Corresponding issue:

Testing

NA

Checklist

  • I have read the contribution guidelines
  • I have targeted this PR against the correct branch (master for website changes, dev for
    source changes)
  • This is either a bugfix, a documentation update, or a new feature that has been explicitly
    approved via an issue
  • I ran the test suite locally (npm run test) and verified that it succeeded

Copy link
Copy Markdown
Collaborator

@alexpetros alexpetros left a comment

Choose a reason for hiding this comment

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

Yup, I know exactly what you're talking about and this fixes it—thank you! If there were a way to specify this config with an environment variable we could put in the package.json and that would be fine, but you're right that we don't want to add another config.

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