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.

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

Add React router tests #430

Merged
merged 4 commits into from
Oct 29, 2024
Merged

Add React router tests #430

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
337d63f
Add React router tests
gabalafou Oct 15, 2024
517192a
Merge branch 'main' into test-memory-router
gabalafou Oct 21, 2024
3b6a1d7
fix bad merge
gabalafou Oct 21, 2024
91b84c5
case-insensitive true
gabalafou Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/layouts/PageLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export const PageLayout = () => {
justifyContent: "center",
height: "100%"
}}
data-testid="no-environment-selected"
>
<Typography sx={{ fontSize: "18px", color: "#333" }}>
Select an environment to show details
Expand Down
32 changes: 16 additions & 16 deletions src/preferences.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,55 +21,55 @@ export interface IPreferences {
urlBasename: string;
}

const { condaStoreConfig = {} } =
typeof window !== "undefined" && (window as any);
let condaStoreConfig: any = {};
if (typeof window !== "undefined" && "condaStoreConfig" in window) {
condaStoreConfig = window.condaStoreConfig;
}

export const prefDefault: Readonly<IPreferences> = {
apiUrl:
process.env.REACT_APP_API_URL ??
condaStoreConfig.REACT_APP_API_URL ??
process.env.REACT_APP_API_URL ??
"http://localhost:8080/conda-store/",

authMethod:
(process.env.REACT_APP_AUTH_METHOD as IPreferences["authMethod"]) ??
(condaStoreConfig.REACT_APP_AUTH_METHOD as IPreferences["authMethod"]) ??
(process.env.REACT_APP_AUTH_METHOD as IPreferences["authMethod"]) ??
"cookie",

authToken:
process.env.REACT_APP_AUTH_TOKEN ??
condaStoreConfig.REACT_APP_AUTH_TOKEN ??
process.env.REACT_APP_AUTH_TOKEN ??
"",

loginUrl:
process.env.REACT_APP_LOGIN_PAGE_URL ??
condaStoreConfig.REACT_APP_LOGIN_PAGE_URL ??
process.env.REACT_APP_LOGIN_PAGE_URL ??
"http://localhost:8080/conda-store/login?next=",

styleType:
process.env.REACT_APP_STYLE_TYPE ??
condaStoreConfig.REACT_APP_STYLE_TYPE ??
process.env.REACT_APP_STYLE_TYPE ??
"green-accent",

showAuthButton: process.env.REACT_APP_SHOW_AUTH_BUTTON
? JSON.parse(process.env.REACT_APP_SHOW_AUTH_BUTTON)
: condaStoreConfig !== undefined &&
condaStoreConfig.REACT_APP_SHOW_AUTH_BUTTON !== undefined
? JSON.parse(condaStoreConfig.REACT_APP_SHOW_AUTH_BUTTON)
: true,
showAuthButton:
(condaStoreConfig.REACT_APP_SHOW_AUTH_BUTTON ??
peytondmurray marked this conversation as resolved.
Show resolved Hide resolved
process.env.REACT_APP_SHOW_AUTH_BUTTON ??
"true") === "true",
peytondmurray marked this conversation as resolved.
Show resolved Hide resolved

logoutUrl:
process.env.REACT_APP_LOGOUT_PAGE_URL ??
condaStoreConfig.REACT_APP_LOGOUT_PAGE_URL ??
process.env.REACT_APP_LOGOUT_PAGE_URL ??
"http://localhost:8080/conda-store/logout?next=/",

routerType:
process.env.REACT_APP_ROUTER_TYPE ??
condaStoreConfig.REACT_APP_ROUTER_TYPE ??
process.env.REACT_APP_ROUTER_TYPE ??
"browser",

urlBasename:
process.env.REACT_APP_URL_BASENAME ??
condaStoreConfig.REACT_APP_URL_BASENAME ??
process.env.REACT_APP_URL_BASENAME ??
"/"
};

Expand Down
12 changes: 12 additions & 0 deletions test/playwright/memory-router-test.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<!doctype html>
<html>
<head>
<script>
window.condaStoreConfig = {
REACT_APP_ROUTER_TYPE: "memory"
};
</script>
</head>
<body>
</body>
</html>
88 changes: 88 additions & 0 deletions test/playwright/test_react_router.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
"""Test app with different types of React routers

- browser router (uses the history API)
- memory router (uses in-app memory)

Ref: https://reactrouter.com/en/main/routers/create-memory-router
"""

import pytest
import re
from playwright.sync_api import Page, expect


@pytest.fixture
def test_config():
return {"base_url": "http://localhost:8000"}


def test_browser_router_200_ok(page: Page, test_config):
"""With browser router, a known route should show the corresponding view
"""
# Check that when going to a known route (in this case, the route to create
# a new environment), the app loads the view for that route.
page.goto(test_config["base_url"] + "/default/new-environment")

# We know we are at the correct view (i.e., new environment form) if there
# is a textbox to enter the name of the new environment.
expect(page.get_by_role("textbox", name="environment name")).to_be_visible()


def test_memory_router_200_ok():
"""With memory router, all routes are 200 (OK) so there's nothing to test there
"""
pass


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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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').

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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",

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. User visits /foo/bar
  2. Server returns React app with 200 status code
  3. 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"
  4. 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.

expect(page.get_by_text("404")).to_be_visible()


def test_memory_router_404_not_found(page: Page, test_config):
"""The memory router has been configured to load the root view at any route
"""
# The route `/memory-router-test.html` is not a route recognized by the
# React app. With the browser router, an unknown route would give a 404.
page.goto(test_config["base_url"] + "/memory-router-test.html")
expect(page.get_by_test_id("no-environment-selected")).to_be_visible()


def test_browser_router_updates_location(page: Page, test_config):
"""With browser router, following a link should update browser URL
"""
# Go to root view and verify that it loaded
page.goto(test_config["base_url"])
expect(page.get_by_test_id("no-environment-selected")).to_be_visible()

# Get and click link to "create new environment"
page.get_by_role("button", name="default").get_by_role(
"link",
# Note the accessible name is determined by the aria-label,
# not the link text
name=re.compile("new.*environment", re.IGNORECASE),
).click()

# With browser router, the window location should update in response to
# clicking an app link
expect(page).to_have_url(re.compile("/default/new-environment"))


def test_memory_router_does_not_update_location(page: Page, test_config):
"""With memory router, following a link should NOT update browser URL
"""
page.goto(test_config["base_url"] + "/memory-router-test.html")

# Get and click link to "create new environment"
page.get_by_role("button", name="default").get_by_role(
"link",
# Note the accessible name is determined by the aria-label,
# not the link text
name=re.compile("new.*environment", re.IGNORECASE),
).click()

# With memory router, the window location should **not** update in response
# to clicking an app link
expect(page).to_have_url(re.compile("/memory-router-test.html"))
10 changes: 7 additions & 3 deletions webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* Copyright (c) 2020, conda-store development team
*
* This file is distributed under the terms of the BSD 3 Clause license.
* This file is distributed under the terms of the BSD 3 Clause license.
* The full license can be found in the LICENSE file.
*/

Expand All @@ -18,7 +18,7 @@ const isProd = process.env.NODE_ENV === "production";
const ASSET_PATH = isProd ? "" : "/";
const version = packageJson.version;

// Calculate hash based on content, will be used when generating production
// Calculate hash based on content, will be used when generating production
// bundles
const cssLoader = {
loader: "css-loader",
Expand Down Expand Up @@ -92,6 +92,10 @@ module.exports = {
},
plugins: [
new HtmlWebpackPlugin({ title: "conda-store" }),
new HtmlWebpackPlugin({
filename: "memory-router-test.html",
template: "test/playwright/memory-router-test.html",
}),
new MiniCssExtractPlugin({
filename: "[name].css",
}),
Expand All @@ -101,7 +105,7 @@ module.exports = {
new webpack.DefinePlugin({
'process.env.VERSION': JSON.stringify(version),
}),
// Add comment to generated files indicating the hash and ui version
// Add comment to generated files indicating the hash and ui version
// this is helpful for vendoring with server
new webpack.BannerPlugin({
banner: `file: [file], fullhash:[fullhash] - ui version: ${version}`,
Expand Down