Add React router tests #430
Conversation
trallard
added
type: enhancement 💅🏼
|
@gabalafou this has some conflicts, can you please fix the branch? @peytondmurray could you please review this PR? |
| def test_browser_router_404_not_found(page: Page, test_config): | ||
| """With browser router, an unknown route should result in a 404 not found error | ||
| """ | ||
| page.goto(test_config["base_url"] + "/this-is-not-an-app-route") |
There was a problem hiding this comment.
Out of curiosity, I tried looking at the Response object returned by page.goto and found that the response code was 200 (OK). Do we expect it to be 404 here? It might be a more robust way of checking the right response, maybe in addition to get_by_text('404').
There was a problem hiding this comment.
Oh, hm, yeah, that is confusing.
I have actually forgotten best practices for this.
I recently created a related conda-store server issue about creating a catch-all route that returns the UI app instead of a 404, if no other route is matched.
I will have to do some research, but I think that ultimately if we want an actual HTTP 404 status code, we will have to define the UI app routes in both the client-side and server-side code.
The only reason these nonsense routes, such as /foo-bar, return the UI app with a 200, by the way, is because of the --history-api-fallback Webpack dev server flag passed to the package.json start script:
"start:ui": "REACT_APP_VERSION=$npm_package_version webpack server --history-api-fallback",
There was a problem hiding this comment.
So the error code itself is intended behavior of --history-api-fallback. Is that good/what we want? Seems like invalid routes should return error codes.
There was a problem hiding this comment.
Hmm? I'm not sure I follow. In the example you pasted above, the --history-api-fallback flag is precisely why /foo/bar returns 200 (with the app bundle) instead of a 404.
I did some web searching and I discovered that one solution (that doesn't require us to find a way to share routes between the front end and the back end) is to do a JavaScript redirect to a 404/not-found page when the user goes to an unknown route. Here's how that would work, step by step:
- User visits /foo/bar
- Server returns React app with 200 status code
- React app boots up in the browser, doesn't recognize the /foo/bar route, executes client-side redirect JavaScript redirect like so:
window.location = "/not-found" - Server returns a 404 page for the /not-found route. (Actually, it could maybe should return the React app at /not-found, but with a 404 status code instead of 200.)
Does that make sense? It's a little less than ideal. But the ideal solution requires us to either duplicate or share the React app routes between both the front-end and back-end codebases.
There was a problem hiding this comment.
Thanks @peytondmurray! I responded to your question and also raised a question about how to handle .env booleans.
| def test_browser_router_404_not_found(page: Page, test_config): | ||
| """With browser router, an unknown route should result in a 404 not found error | ||
| """ | ||
| page.goto(test_config["base_url"] + "/this-is-not-an-app-route") |
There was a problem hiding this comment.
Oh, hm, yeah, that is confusing.
I have actually forgotten best practices for this.
I recently created a related conda-store server issue about creating a catch-all route that returns the UI app instead of a 404, if no other route is matched.
I will have to do some research, but I think that ultimately if we want an actual HTTP 404 status code, we will have to define the UI app routes in both the client-side and server-side code.
The only reason these nonsense routes, such as /foo-bar, return the UI app with a 200, by the way, is because of the --history-api-fallback Webpack dev server flag passed to the package.json start script:
"start:ui": "REACT_APP_VERSION=$npm_package_version webpack server --history-api-fallback",
Note to reviewer: before reviewing this PR, check out the comments on gabalafou#1.
This PR contains the tests for #429.
It is a separate PR because it makes significant changes to the React app code.
I believe that these changes are not only necessary to enable these tests but that they also fix a long-standing bug that I guess we did not even realize that we had.
The bug is that we say in our docs that the UI app can be configured "at runtime, using condaStoreConfig." But before this PR, the app code would choose the build-time over run-time config. It would read the config sources in the following order:
condaStoreConfigdictionary in browserBut that seemed backward and was preventing me from taking the testing approach in this PR, so I swapped the order:
condaStoreConfigDescription
This pull request:
condaStoreConfig, then fromprocess.env(environment variables). Important note: environment variables can only be read at build time.Pull request checklist