diff --git a/.eslintrc.js b/.eslintrc.js index 8dbbf4526..1877e9ee0 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -2,12 +2,16 @@ module.exports = { "env": { "browser": true, "jquery": true, - "es6": true + "es6": true, + "jest/globals": true }, "extends": "eslint:recommended", "parserOptions": { "sourceType": "module" }, "rules": { - } + }, + "plugins": [ + "jest" + ] }; diff --git a/.github/workflows/jest.yml b/.github/workflows/jest.yml new file mode 100644 index 000000000..ea88bae6d --- /dev/null +++ b/.github/workflows/jest.yml @@ -0,0 +1,36 @@ +# Runs jest based unit tests for frontend javascript and @jupyterhub/binderhub-client +name: "JS Unit tests" + +on: + pull_request: + paths: + - "binderhub/static/js/**" + - "js/packages/binderhub-client/**" + - ".github/workflows/jest.yml" + push: + paths: + - "binderhub/static/js/**" + - "js/packages/binderhub-client/**" + - ".github/workflows/jest.yml" + branches-ignore: + - "dependabot/**" + - "pre-commit-ci-update-config" + - "update-*" + workflow_dispatch: + +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: "Setup dependencies" + run: | + npm install + + - name: "Run all unit tests" + run: | + npm test + + - name: Upload coverage to Codecov + uses: codecov/codecov-action@v3 diff --git a/binderhub/static/js/index.js b/binderhub/static/js/index.js index 57eebf393..3d1b61963 100644 --- a/binderhub/static/js/index.js +++ b/binderhub/static/js/index.js @@ -19,6 +19,7 @@ import { BinderRepository } from '@jupyterhub/binderhub-client'; import { makeBadgeMarkup } from './src/badge'; import { getPathType, updatePathText } from './src/path'; import { nextHelpText } from './src/loading'; +import { updateFavicon } from './src/favicon'; import 'xterm/css/xterm.css'; @@ -33,14 +34,6 @@ const BASE_URL = $('#base-url').data().url; const BADGE_BASE_URL = $('#badge-base-url').data().url; let config_dict = {}; -function update_favicon(path) { - let link = document.querySelector("link[rel*='icon']") || document.createElement('link'); - link.type = 'image/x-icon'; - link.rel = 'shortcut icon'; - link.href = path; - document.getElementsByTagName('head')[0].appendChild(link); -} - function v2url(providerPrefix, repository, ref, path, pathType) { // return a v2 url from a providerPrefix, repository, ref, and (file|url)path if (repository.length === 0) { @@ -153,7 +146,7 @@ function updateUrls(formValues) { } function build(providerSpec, log, fitAddon, path, pathType) { - update_favicon(BASE_URL + "favicon_building.ico"); + updateFavicon(BASE_URL + "favicon_building.ico"); // split provider prefix off of providerSpec const spec = providerSpec.slice(providerSpec.indexOf('/') + 1); // Update the text of the loading page if it exists @@ -167,7 +160,11 @@ function build(providerSpec, log, fitAddon, path, pathType) { $('.on-build').removeClass('hidden'); const buildToken = $("#build-token").data('token'); - const image = new BinderRepository(providerSpec, BASE_URL, buildToken); + // If BASE_URL is absolute, use that as the base for build endpoint URL. + // Else, first resolve BASE_URL relative to current URL, then use *that* as the + // base for the build endpoint url. + const buildEndpointUrl = new URL("build", new URL(BASE_URL, window.location.href)); + const image = new BinderRepository(providerSpec, buildEndpointUrl, buildToken); image.onStateChange('*', function(oldState, newState, data) { if (data.message !== undefined) { @@ -198,7 +195,7 @@ function build(providerSpec, log, fitAddon, path, pathType) { $("#loader").addClass("paused"); // If we fail for any reason, show an error message and logs - update_favicon(BASE_URL + "favicon_fail.ico"); + updateFavicon(BASE_URL + "favicon_fail.ico"); log.show(); if ($('div#loader-text').length > 0) { $('#loader').addClass("error"); @@ -213,13 +210,16 @@ function build(providerSpec, log, fitAddon, path, pathType) { $('#phase-launching').removeClass('hidden'); } $('#phase-launching').removeClass('hidden'); - update_favicon(BASE_URL + "favicon_success.ico"); + updateFavicon(BASE_URL + "favicon_success.ico"); }); image.onStateChange('ready', function(oldState, newState, data) { image.close(); + // If data.url is an absolute URL, it'll be used. Else, it'll be interpreted + // relative to current page's URL. + const serverUrl = new URL(data.url, window.location.href); // user server is ready, redirect to there - image.launch(data.url, data.token, path, pathType); + window.location.href = image.getFullRedirectURL(serverUrl, data.token, path, pathType); }); image.fetch(); diff --git a/binderhub/static/js/src/favicon.js b/binderhub/static/js/src/favicon.js new file mode 100644 index 000000000..1353226dc --- /dev/null +++ b/binderhub/static/js/src/favicon.js @@ -0,0 +1,17 @@ +/** + * Dynamically set current page's favicon. + * + * @param {String} href Path to Favicon to use + */ +function updateFavicon(href) { + let link = document.querySelector("link[rel*='icon']"); + if(!link) { + link = document.createElement('link'); + document.getElementsByTagName('head')[0].appendChild(link); + } + link.type = 'image/x-icon'; + link.rel = 'shortcut icon'; + link.href = href; +} + +export { updateFavicon }; diff --git a/binderhub/static/js/src/favicon.test.js b/binderhub/static/js/src/favicon.test.js new file mode 100644 index 000000000..dfd515d3a --- /dev/null +++ b/binderhub/static/js/src/favicon.test.js @@ -0,0 +1,29 @@ +import { updateFavicon } from "./favicon"; + +afterEach(() => { + // Clear out HEAD after each test run, so our DOM is clean. + // Jest does *not* clear out the DOM between test runs on the same file! + document.querySelector("head").innerHTML = ''; +}); + +test("Setting favicon when there is none works", () => { + expect(document.querySelector("link[rel*='icon']")).toBeNull(); + + updateFavicon("https://example.com/somefile.png"); + + expect(document.querySelector("link[rel*='icon']").href).toBe("https://example.com/somefile.png"); +}); + +test("Setting favicon multiple times works without leaking link tags", () => { + expect(document.querySelector("link[rel*='icon']")).toBeNull(); + + updateFavicon("https://example.com/somefile.png"); + + expect(document.querySelector("link[rel*='icon']").href).toBe("https://example.com/somefile.png"); + expect(document.querySelectorAll("link[rel*='icon']").length).toBe(1); + + updateFavicon("https://example.com/some-other-file.png"); + + expect(document.querySelector("link[rel*='icon']").href).toBe("https://example.com/some-other-file.png"); + expect(document.querySelectorAll("link[rel*='icon']").length).toBe(1); +}); diff --git a/js/packages/binderhub-client/lib/index.js b/js/packages/binderhub-client/lib/index.js index 19d239304..4261e4a7a 100644 --- a/js/packages/binderhub-client/lib/index.js +++ b/js/packages/binderhub-client/lib/index.js @@ -10,13 +10,28 @@ export class BinderRepository { /** * * @param {string} providerSpec Spec of the form // to pass to the binderhub API. - * @param {string} baseUrl Base URL (including the trailing slash) of the binderhub installation to talk to. - * @param {string} buildToken Optional JWT based build token if this binderhub installation requires using build tokesn + * @param {URL} buildEndpointUrl API URL of the build endpoint to talk to + * @param {string} buildToken Optional JWT based build token if this binderhub installation requires using build tokens */ - constructor(providerSpec, baseUrl, buildToken) { + constructor(providerSpec, buildEndpointUrl, buildToken) { this.providerSpec = providerSpec; - this.baseUrl = baseUrl; - this.buildToken = buildToken; + // Make sure that buildEndpointUrl is a real URL - this ensures hostname is properly set + if(!(buildEndpointUrl instanceof URL)) { + throw new TypeError(`buildEndpointUrl must be a URL object, got ${buildEndpointUrl} instead`); + } + // We make a copy here so we don't modify the passed in URL object + this.buildEndpointUrl = new URL(buildEndpointUrl); + // The binderHub API is path based, so the buildEndpointUrl must have a trailing slash. We add + // it if it is not passed in here to us. + if(!this.buildEndpointUrl.pathname.endsWith('/')) { + this.buildEndpointUrl.pathname += "/"; + } + + // The actual URL we'll make a request to build this particular providerSpec + this.buildUrl = new URL(this.providerSpec, this.buildEndpointUrl); + if(buildToken) { + this.buildUrl.searchParams.append("build_token", buildToken); + } this.callbacks = {}; this.state = null; } @@ -25,12 +40,7 @@ export class BinderRepository { * Call the BinderHub API */ fetch() { - let apiUrl = this.baseUrl + "build/" + this.providerSpec; - if (this.buildToken) { - apiUrl = apiUrl + `?build_token=${this.buildToken}`; - } - - this.eventSource = new EventSource(apiUrl); + this.eventSource = new EventSource(this.buildUrl); this.eventSource.onerror = (err) => { console.error("Failed to construct event stream", err); this._changeState("failed", { @@ -59,38 +69,48 @@ export class BinderRepository { } /** - * Redirect user to a running jupyter server with given token + * Get URL to redirect user to on a Jupyter Server to display a given path - * @param {URL} url URL to the running jupyter server + * @param {URL} serverUrl URL to the running jupyter server * @param {string} token Secret token used to authenticate to the jupyter server * @param {string} path The path of the file or url suffix to launch the user into * @param {string} pathType One of "lab", "file" or "url", denoting what kinda path we are launching the user into + * + * @returns {URL} A URL to redirect the user to */ - launch(url, token, path, pathType) { - // redirect a user to a running server with a token + getFullRedirectURL(serverUrl, token, path, pathType) { + // Make a copy of the URL so we don't mangle the original + let url = new URL(serverUrl); if (path) { - // strip trailing / - url = url.replace(/\/$/, ""); - // trim leading '/' + // strip trailing / from URL + url.pathname = url.pathname.replace(/\/$/, ""); + + // trim leading '/' from path to launch users into path = path.replace(/(^\/)/g, ""); + if (pathType === "lab") { + // The path is a specific *file* we should open with JupyterLab + // trim trailing / on file paths path = path.replace(/(\/$)/g, ""); + // /doc/tree is safe because it allows redirect to files - url = url + "/doc/tree/" + encodeURI(path); + url.pathname = url.pathname + "/doc/tree/" + encodeURI(path); } else if (pathType === "file") { + // The path is a specific file we should open with *classic notebook* + // trim trailing / on file paths path = path.replace(/(\/$)/g, ""); // /tree is safe because it allows redirect to files - url = url + "/tree/" + encodeURI(path); + url.pathname = url.pathname + "/tree/" + encodeURI(path); } else { - // pathType === 'url' - url = url + "/" + path; + // pathType is 'url' and we should just pass it on + url.pathname = url.pathname + "/" + path; } } - const sep = url.indexOf("?") == -1 ? "?" : "&"; - url = url + sep + $.param({ token: token }); - window.location.href = url; + + url.searchParams.append('token', token); + return url; } diff --git a/js/packages/binderhub-client/lib/index.test.js b/js/packages/binderhub-client/lib/index.test.js new file mode 100644 index 000000000..7fd96c445 --- /dev/null +++ b/js/packages/binderhub-client/lib/index.test.js @@ -0,0 +1,157 @@ +import { BinderRepository } from "."; + +test("Passed in URL object is not modified", () => { + const buildEndpointUrl = new URL("https://test-binder.org/build"); + const br = new BinderRepository("gh/test/test", buildEndpointUrl, "token"); + expect(br.buildEndpointUrl.toString()).not.toEqual( + buildEndpointUrl.toString() + ); +}); + +test("Invalid URL errors out", () => { + expect(() => { + new BinderRepository("gh/test/test", "/build", "token"); + }).toThrow(TypeError); +}); + +test("Trailing slash added if needed", () => { + const buildEndpointUrl = new URL("https://test-binder.org/build"); + const br = new BinderRepository("gh/test/test", buildEndpointUrl); + expect(br.buildEndpointUrl.toString()).toEqual( + "https://test-binder.org/build/" + ); +}); + +test("Build URL correctly built from Build Endpoint", () => { + const buildEndpointUrl = new URL("https://test-binder.org/build"); + const br = new BinderRepository("gh/test/test", buildEndpointUrl); + expect(br.buildUrl.toString()).toEqual( + "https://test-binder.org/build/gh/test/test" + ); +}); + +test("Build URL correctly built from Build Endpoint when used with token", () => { + const buildEndpointUrl = new URL("https://test-binder.org/build"); + const br = new BinderRepository("gh/test/test", buildEndpointUrl, "token"); + expect(br.buildUrl.toString()).toEqual( + "https://test-binder.org/build/gh/test/test?build_token=token" + ); +}); + +test("Get full redirect URL with correct token but no path", () => { + const br = new BinderRepository( + "gh/test/test", + new URL("https://test-binder.org/build") + ); + expect( + br + .getFullRedirectURL("https://hub.test-binder.org/user/something", "token") + .toString() + ).toBe("https://hub.test-binder.org/user/something?token=token"); +}); + +test("Get full redirect URL with urlpath", () => { + const br = new BinderRepository( + "gh/test/test", + new URL("https://test-binder.org/build") + ); + expect( + br + .getFullRedirectURL( + "https://hub.test-binder.org/user/something", + "token", + "rstudio", + "url" + ) + .toString() + ).toBe("https://hub.test-binder.org/user/something/rstudio?token=token"); +}); + +test("Get full redirect URL when opening a file with jupyterlab", () => { + const br = new BinderRepository( + "gh/test/test", + new URL("https://test-binder.org/build") + ); + expect( + br + .getFullRedirectURL( + "https://hub.test-binder.org/user/something", + "token", + "index.ipynb", + "lab" + ) + .toString() + ).toBe("https://hub.test-binder.org/user/something/doc/tree/index.ipynb?token=token"); +}); + +test("Get full redirect URL when opening a file with classic notebook (with file= path)", () => { + const br = new BinderRepository( + "gh/test/test", + new URL("https://test-binder.org/build") + ); + expect( + br + .getFullRedirectURL( + "https://hub.test-binder.org/user/something", + "token", + "index.ipynb", + "file" + ) + .toString() + ).toBe("https://hub.test-binder.org/user/something/tree/index.ipynb?token=token"); +}); + +test("Get full redirect URL and deal with excessive slashes (with pathType=url)", () => { + const br = new BinderRepository( + "gh/test/test", + new URL("https://test-binder.org/build") + ); + expect( + br + .getFullRedirectURL( + // Trailing slash should not be preserved here + "https://hub.test-binder.org/user/something/", + "token", + // Trailing slash should be preserved here, but leading slash should not be repeated + "/rstudio/", + "url" + ) + .toString() + ).toBe("https://hub.test-binder.org/user/something/rstudio/?token=token"); +}); + +test("Get full redirect URL and deal with excessive slashes (with pathType=lab)", () => { + const br = new BinderRepository( + "gh/test/test", + new URL("https://test-binder.org/build") + ); + expect( + br + .getFullRedirectURL( + "https://hub.test-binder.org/user/something/", + "token", + // Both leading and trailing slashes should be gone here. + "/directory/index.ipynb/", + "lab" + ) + .toString() + ).toBe("https://hub.test-binder.org/user/something/doc/tree/directory/index.ipynb?token=token"); +}); + +test("Get full redirect URL and deal with excessive slashes (with pathType=file)", () => { + const br = new BinderRepository( + "gh/test/test", + new URL("https://test-binder.org/build") + ); + expect( + br + .getFullRedirectURL( + "https://hub.test-binder.org/user/something/", + "token", + // Both leading and trailing slashes should be gone here. + "/directory/index.ipynb/", + "file" + ) + .toString() + ).toBe("https://hub.test-binder.org/user/something/tree/directory/index.ipynb?token=token"); +}); diff --git a/package.json b/package.json index cfdaeacc6..2f0aa5b64 100644 --- a/package.json +++ b/package.json @@ -13,9 +13,14 @@ "@babel/cli": "^7.21.0", "@babel/core": "^7.21.4", "@babel/preset-env": "^7.21.4", + "@types/jest": "^29.5.5", + "babel-jest": "^29.7.0", "babel-loader": "^9.1.2", "css-loader": "^6.7.3", "eslint": "^8.38.0", + "eslint-plugin-jest": "^27.4.2", + "jest": "^29.7.0", + "jest-environment-jsdom": "^29.7.0", "mini-css-extract-plugin": "^2.7.5", "webpack": "^5.78.0", "webpack-cli": "^5.0.1" @@ -26,6 +31,15 @@ "scripts": { "webpack": "webpack", "webpack:watch": "webpack --watch", - "lint": "eslint binderhub/static/js" + "lint": "eslint binderhub/static/js", + "test": "jest" + }, + "jest": { + "testEnvironment": "jsdom", + "collectCoverage": true, + "coverageReporters": [ + "text", + "cobertura" + ] } }