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 React router tests #1

Open
wants to merge 6 commits into
base: fix-for-jupyterlab
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ REACT_APP_STYLE_TYPE=green-accent
REACT_APP_CONTEXT=webapp
REACT_APP_SHOW_AUTH_BUTTON=true
REACT_APP_LOGOUT_PAGE_URL=http://localhost:8080/conda-store/logout?next=/
REACT_APP_ROUTER_TYPE=browser

# If you need to mount the React app at some URL path other than "/". This value
# is passed directly to React Router, see:
# https://reactrouter.com/en/main/routers/create-browser-router#optsbasename
# REACT_APP_URL_BASENAME="/conda-store"

# If you want to use a version other than the pinned conda-store-server version
# Set the CONDA_STORE_SERVER_VERSION to the package version that you want
Expand Down
17 changes: 14 additions & 3 deletions src/App.tsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
import { ThemeProvider } from "@mui/material";
import React from "react";
import { Provider } from "react-redux";
import { RouterProvider } from "react-router-dom";

import {
RouterProvider,
createBrowserRouter,
createMemoryRouter
} from "react-router-dom";
import {
IPreferences,
PrefContext,
prefDefault,
prefGlobal
} from "./preferences";
import { router } from "./routes";
import { routes } from "./routes";

import { store } from "./store";
import { condaStoreTheme, grayscaleTheme } from "./theme";

Expand Down Expand Up @@ -64,6 +68,13 @@ export class App<
// }

render(): React.ReactNode {
const { routerType, urlBasename: basename } = this.state.pref;

const router =
routerType === "memory"
? createMemoryRouter(routes, { initialEntries: ["/"] })
: createBrowserRouter(routes, { basename });

return (
<PrefContext.Provider value={this.state.pref}>
<ThemeProvider
Expand Down
1 change: 1 addition & 0 deletions src/layouts/PageLayout.tsx
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
66 changes: 50 additions & 16 deletions src/preferences.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,48 +8,70 @@ export interface IPreferences {
styleType: string;
showAuthButton: boolean;
logoutUrl: string;

// routerType - Should the app use the browser's history API for routing, or
// should app routes be handled internally in memory? This is needed for the
// JupyterLab extension because when conda-store-ui is embedded in JupyterLab,
// the URL routes in the browser address bar are for JupyterLab, not for
// conda-store-ui.
routerType: "browser" | "memory";

// urlBasename - Defaults to "/" but can be changed if the app needs to be
// mounted at a different URL path, such as "/conda-store"
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 ??
Copy link
Owner Author

@gabalafou gabalafou Oct 15, 2024

Choose a reason for hiding this comment

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

The Conda Store docs say that condaStoreConfig can be used to configure the UI app at runtime.

I take this to also imply that run-time should override build-time configuration.

If not, it is impossible to set a config variable in the way that I do in the memory-router-test.html file in this PR if that variable was already defined as an environment variable at build time.

So for example, if in my .env file I have:

REACT_APP_ROUTER_TYPE=browser

And then in my app index html page I have:

<script>
window.condaStoreConfig = {
  REACT_APP_ROUTER_TYPE: "memory"
};
<script>

Then the app will actually be loaded with the browser router and not the memory router. I don't think that's what we want, but I may be mistaken.

Choose a reason for hiding this comment

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

Runtime indeed overwrites/supersedes build time.
This is now how we are passing default config values to conda-store-server vs the hack we used to have before https://github.com/conda-incubator/conda-store/blob/d39e5b47577aa6764b1bf99a0ab675fe8782e969/conda-store-server/conda_store_server/_internal/server/templates/conda-store-ui.html#L11

Copy link
Owner Author

@gabalafou gabalafou Oct 15, 2024

Choose a reason for hiding this comment

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

That's interesting. Does that file you sent actually work? Because as far as I can tell, the only way it would work is if the main.js file at static/conda-store-ui/main.js were built in an environment in which none of those REACT_APP_-prefixed variables were defined at build time. I searched the UI codebase for condaStoreConfig to see if I was missing something, but the only place that variable is read is in the preferences.tsx file, and in that file, it's pretty clear that the variable from condaStoreConfig is only read if the corresponding environment variable were nullish at build-time.

For example, let's look at the following JavaScript:

process.env.REACT_APP_API_URL ??
condaStoreConfig.REACT_APP_API_URL ??
"http://localhost:8080/conda-store/"

At build time Webpack will do a string substitution and replace process.env.REACT_APP_API_URL with the value in the environment, if it's defined. So, if it's set to "http://localhost:8080/conda-store/", then the above JavaScript is transformed to:

"http://localhost:8080/conda-store/" ??
condaStoreConfig.REACT_APP_API_URL ??
"http://localhost:8080/conda-store/"

Since the first argument in the above expression is not "nullish", meaning neither null nor undefined, then the overall expression reduces to that first argument, i.e.:

"http://localhost:8080/conda-store/"

On the other hand, if that environment variable is not defined at build time, the source code is transformed as:

undefined ??
condaStoreConfig.REACT_APP_API_URL ??
"http://localhost:8080/conda-store/"

Since the left hand side of the nullish operator is undefined the runtime will try to read the value from condaStoreConfig, and only if that too is null or undefined, will it return the last operand.

So the only way the condaStoreConfig variable plays any role at run time is if the corresponding environment variable was not defined at build time.

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"]) ??
condaStoreConfig.REACT_APP_AUTH_METHOD ??
process.env.REACT_APP_AUTH_METHOD ??
"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: /true/i.test(
condaStoreConfig.REACT_APP_SHOW_AUTH_BUTTON ??
process.env.REACT_APP_SHOW_AUTH_BUTTON ??
"true"
),

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

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

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

export class Preferences implements IPreferences {
Expand Down Expand Up @@ -85,6 +107,14 @@ export class Preferences implements IPreferences {
return this._logoutUrl;
}

get routerType() {
return this._routerType;
}

get urlBasename() {
return this._urlBasename;
}

set(pref: IPreferences) {
this._apiUrl = pref.apiUrl;
this._authMethod = pref.authMethod;
Expand All @@ -93,6 +123,8 @@ export class Preferences implements IPreferences {
this._styleType = pref.styleType;
this._showAuthButton = pref.showAuthButton;
this._logoutUrl = pref.logoutUrl;
this._routerType = pref.routerType;
this._urlBasename = pref.urlBasename;
}

private _apiUrl: IPreferences["apiUrl"];
Expand All @@ -102,6 +134,8 @@ export class Preferences implements IPreferences {
private _styleType: IPreferences["styleType"];
private _showAuthButton: IPreferences["showAuthButton"];
private _logoutUrl: IPreferences["logoutUrl"];
private _routerType: IPreferences["routerType"];
private _urlBasename: IPreferences["urlBasename"];
}

export const prefGlobal = new Preferences();
Expand Down
6 changes: 3 additions & 3 deletions src/routes.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import React from "react";
import { createBrowserRouter } from "react-router-dom";

import { PageLayout } from "./layouts";
import { EnvironmentDetails } from "./features/environmentDetails";
Expand All @@ -8,7 +7,8 @@ import { EnvironmentCreate } from "./features/environmentCreate";
/**
* Define URL routes for the single page app
*/
export const router = createBrowserRouter([

export const routes = [
{
path: "/",
element: <PageLayout />,
Expand All @@ -23,4 +23,4 @@ export const router = createBrowserRouter([
}
]
}
]);
];
12 changes: 12 additions & 0 deletions test/playwright/memory-router-test.html
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
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")
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()
Comment on lines +44 to +50

Choose a reason for hiding this comment

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

So basically, do non-existing routes always redirect to /conda-store?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Not exactly. The memory router does not redirect anywhere. It loads the app at the route you pre-configure. For example, in my other PR, I configure the memory router to start with the root route. That means that no matter what the URL in the browser address bar says, if the app is loaded at that URL, it will load the root view.

The browser router, by contrast, tries to read the URL path from the browser address bar when the app loads and then render the app view that matches that route (or render a 404 if no view matches the route).

In other words, the browser router is URL-aware, whereas the memory router is not. That's the main difference.



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