Skip to content
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.

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

Add test cases to validate functionality of PUBLIC_URL and relative homepage #1516

Closed
Timer opened this issue Feb 10, 2017 · 12 comments
Closed

Comments

@Timer
Copy link
Contributor

Timer commented Feb 10, 2017

We recently added PUBLIC_URL support and official . (relative) homepage support.

I think it's very important to test these behaviors to ensure they're being properly applied.
PUBLIC_URL is already partially tested but could be improved, and there exists no test for relative homepage support.

#1519 is the first step of this, we need much deeper integration testing (with other assets, maybe include bootstrap).

@Timer Timer changed the title Add test cases to validate functionality of PUBLIC_URL and . homepage Add test cases to validate functionality of PUBLIC_URL and relative homepage Feb 10, 2017
@EnoahNetzach
Copy link
Contributor

The test runner would have to

  • create a new project (the simplest one), or
  • create a single project and modify the "homepage" field

for each test case.

A way to test them could be that of compiling them in various cases, and look for usages in the code afterward (similar to what is done here.)

This way could be really demanding for travis' CI timewise, though..
Tests could be conducted either in "installs" or in "kitchensink", although the latter is currently the slowest (in node 4), or in a separate "e2e" test case altogether.

Another open question is: should there be a difference between production and development, therefore tested?

@Timer
Copy link
Contributor Author

Timer commented Feb 10, 2017

That's exactly what I had in mind, so it's great to hear that you're on the same page @EnoahNetzach.

I think testing this case warrants some extra CI time, but I believe it's something we only have to test on Node 6.

Also, we removed the PUBLIC_URL effect in development so it remained the same as before. I want to test that it's not respected.

@EnoahNetzach
Copy link
Contributor

Probably the long execution of "kitchensink" in node 4 lets us add another job to the travis' matrix, dedicated to this (and maybe future related) problems.
It would still finish before that long one.

I'd try it creating a new project each time (similar to "installs"), and see how much it takes..

For testing I think a simple sed would mostly work (?)

Which are the test cases, and the expected outputs?

For the PUBLIC_URL case these are:

  • full URL: https://www.example.org/cra should remain the same
  • URI: /cra should remain the same

For the "homepage" case:

  • full URL: https://www.example.org/cra -> /cra
  • URI: /cra should remain the same
  • ".": should be a relative path

Am I right?

@Timer
Copy link
Contributor Author

Timer commented Feb 10, 2017

Correct @EnoahNetzach -- but we should also check the PUBLIC_URL cases when the "homepage" is set opposite of what we're testing to make sure that PUBLIC_URL overrides it.

Checking "." should be as simple as: grep -R -q "../../static/" build/ and checking the exit code. Others similarly. We should also check that some greps are false, such as "/static and '/static (to make sure it was done everywhere and not just in one case that our test matched).

@Timer
Copy link
Contributor Author

Timer commented Feb 10, 2017

Are you giving this a whirl, @EnoahNetzach? Or should I try to pull something together?

@Timer
Copy link
Contributor Author

Timer commented Feb 10, 2017

Started #1519. Feel free to contribute.

@EnoahNetzach
Copy link
Contributor

@Timer you go, I'll watch and judge 😛

@Timer
Copy link
Contributor Author

Timer commented Feb 10, 2017

I'd love a review on #1519 @EnoahNetzach.

@EnoahNetzach
Copy link
Contributor

@Timer tomorrow I'll be more than happy

@gaearon
Copy link
Contributor

gaearon commented Mar 7, 2017

What's left here?

@Timer
Copy link
Contributor Author

Timer commented Mar 7, 2017

The bash implementation isn't preferable, we would probably benefit more from a more comprehensive entry in kitchensink that uses jest.

@gaearon
Copy link
Contributor

gaearon commented Jan 8, 2018

Closing as stale.

@gaearon gaearon closed this as completed Jan 8, 2018
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants