From 29c210032e750d6bff1d41910434b8605eebc0f6 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Wed, 13 Jan 2021 17:41:49 -0800 Subject: [PATCH 1/3] ci: backport new CI to 2.4 release branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Generated with `git restore -s origin/master .travis.yml .github/ ci/`, plus the following manual changes: - removed the `build-data-server-pip` CI job, since 2.4 doesn’t have any Rust packages to build; - reinstated `flake8` for Python 3.5; - removed CI check for absence of `"@npm_angular_bazel//:index.bzl"`; - downgraded CI Bazel to 2.1.0 to match WORKSPACE file; - cherry-picked `test_pip_package.sh` changes for `--tf-version notf`. Test Plan: CI test run passed: wchargin-branch: ci-backport-2.4 wchargin-source: a6833768fb4e5c516f426d5c32b4044063fcfefc --- .github/workflows/ci.yml | 142 +++++++++++++------- .travis.yml | 115 ---------------- ci/bazelrc | 26 ++-- tensorboard/pip_package/test_pip_package.sh | 5 +- 4 files changed, 114 insertions(+), 174 deletions(-) delete mode 100644 .travis.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3d778456d6..9d6a1df84d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,9 +6,6 @@ # Helpful YAML parser to clarify YAML syntax: # https://yaml-online-parser.appspot.com/ -# For now, we only use GitHub Actions for lint checks, pending better -# support for hermetic-style caching. See: -# https://github.com/actions/cache/issues/109 name: CI on: @@ -18,13 +15,94 @@ on: - '[0-9]+.*' - 'ci-*' pull_request: {} + schedule: + # 13:00 UTC is 05:00 in Pacific standard time (UTC-8), which is well + # after nightly TensorFlow wheels are released (around 2--3 AM) and + # just after nightly TensorBoard wheels are released (around 04:15). + # (cron syntax: minute hour day-of-month month day-of-week) + - cron: '0 13 * * *' env: + # Keep this Bazel version in sync with the `versions.check` directive + # in our WORKSPACE file. + BAZEL_VERSION: '2.1.0' + BAZEL_SHA256SUM: 'e13581d44faad6ac807dd917e682fef20359d26728166ac35dadd8ee653a580d' BUILDTOOLS_VERSION: '3.0.0' BUILDIFIER_SHA256SUM: 'e92a6793c7134c5431c58fbc34700664f101e5c9b1c1fcd93b97978e8b7f88db' BUILDOZER_SHA256SUM: '3d58a0b6972e4535718cdd6c12778170ea7382de7c75bc3728f5719437ffb84d' jobs: + build: + runs-on: ubuntu-16.04 + needs: lint-python-flake8 # fail fast in case of "undefined variable" errors + strategy: + fail-fast: false + matrix: + tf_version_id: ['tf-nightly', 'notf'] + python_version: ['3.7'] + steps: + - uses: actions/checkout@v1 + - uses: actions/setup-python@v1 + with: + python-version: ${{ matrix.python_version }} + architecture: 'x64' + - name: 'Set up Bazel' + run: | + ci/download_bazel.sh "${BAZEL_VERSION}" "${BAZEL_SHA256SUM}" ~/bazel + sudo mv ~/bazel /usr/local/bin/bazel + sudo chmod +x /usr/local/bin/bazel + cp ./ci/bazelrc ~/.bazelrc + - name: 'Configure build cache write credentials' + env: + CREDS: ${{ secrets.BAZEL_CACHE_SERVICE_ACCOUNT_CREDS }} + EVENT_TYPE: ${{ github.event_name }} + run: | + if [ -z "${CREDS}" ]; then + printf 'Using read-only cache (no credentials)\n' + exit + fi + if [ "${EVENT_TYPE}" = pull_request ]; then + printf 'Using read-only cache (PR build)\n' + exit + fi + printf 'Using writable cache\n' + creds_file=/tmp/service_account_creds.json + printf '%s\n' "${CREDS}" >"${creds_file}" + printf '%s\n' >>~/.bazelrc \ + "common --google_credentials=${creds_file}" \ + "common --remote_upload_local_results=true" \ + ; + - name: 'Install Python dependencies' + run: | + python -m pip install -U pip + pip install \ + -r ./tensorboard/pip_package/requirements.txt \ + -r ./tensorboard/pip_package/requirements_dev.txt \ + ; + - name: 'Install TensorFlow' + run: pip install "${{ matrix.tf_version_id }}" + if: matrix.tf_version_id != 'notf' + - name: 'Check Pip state' + run: pip freeze --all + - name: 'Bazel: fetch' + run: bazel fetch //tensorboard/... + - name: 'Bazel: build' + run: bazel build //tensorboard/... + - name: 'Bazel: test (with TensorFlow support)' + run: bazel test //tensorboard/... + if: matrix.tf_version_id != 'notf' + - name: 'Bazel: test (non-TensorFlow only)' + run: bazel test //tensorboard/... --test_tag_filters="support_notf" + if: matrix.tf_version_id == 'notf' + - name: 'Bazel: run Pip package test' + run: | + bazel run //tensorboard/pip_package:test_pip_package -- \ + --tf-version "${{ matrix.tf_version_id }}" + - name: 'Bazel: run manual tests' + run: | + bazel test //tensorboard/compat/tensorflow_stub:gfile_s3_test && + bazel test //tensorboard/summary/writer:event_file_writer_s3_test + lint-python-flake8: runs-on: ubuntu-16.04 strategy: @@ -50,7 +128,7 @@ jobs: # Use the comment '# noqa: ' to suppress. run: flake8 . --count --select=E9,F63,F7,F82,F401 --show-source --statistics - lint-python: + lint-python-yaml-docs: runs-on: ubuntu-16.04 steps: - uses: actions/checkout@v1 @@ -58,24 +136,31 @@ jobs: with: python-version: '3.6' architecture: 'x64' - - name: 'Install black' + - name: 'Install black, yamllint, and the TensorFlow docs notebook tools' run: | python -m pip install -U pip - pip install black -c ./tensorboard/pip_package/requirements_dev.txt + nbfmt_version="174c9a5c1cc51a3af1de98d84824c811ecd49029" + pip install black yamllint -c ./tensorboard/pip_package/requirements_dev.txt + pip install -U git+https://github.com/tensorflow/docs@${nbfmt_version} - run: pip freeze --all - name: 'Lint Python code for style with Black' # You can run `black .` to fix all Black complaints. run: black --check --diff . + - name: 'Lint YAML for gotchas with yamllint' + # Use '# yamllint disable-line rule:foo' to suppress. + run: yamllint -c docs/.yamllint docs docs/.yamllint + - name: 'Lint Colab notebooks for formatting with nbfmt' + run: git ls-files -z '*.ipynb' | xargs -0 python3 -m tensorflow_docs.tools.nbfmt --test lint-rust: runs-on: ubuntu-16.04 strategy: matrix: - rust_version: ['1.47.0'] + rust_version: ['1.48.0'] cargo_raze_version: ['0.7.0'] steps: - uses: actions/checkout@v1 - - name: 'Cache Cargo home directory' + - name: 'Cache Cargo artifacts' uses: actions/cache@v2 with: path: | @@ -88,7 +173,7 @@ jobs: # Needed for installing binaries (`cargo-raze`) with cache ~/.cargo/.crates.toml ~/.cargo/.crates2.json - key: ${{ runner.os }}-cargo-${{ matrix.rust_version }}-${{ matrix.cargo_raze_version }}-${{ hashFiles('**/Cargo.lock', '.github/workflows/ci.yml') }} + key: lint-rust-${{ runner.os }}-cargo-${{ matrix.rust_version }}-${{ matrix.cargo_raze_version }}-${{ hashFiles('**/Cargo.lock', '.github/workflows/ci.yml') }} - name: 'Install Rust toolchain' uses: actions-rs/toolchain@v1 with: @@ -101,37 +186,14 @@ jobs: uses: actions-rs/clippy-check@v1 with: token: ${{ secrets.GITHUB_TOKEN }} - args: --manifest-path tensorboard/data/server/Cargo.toml + args: --tests --manifest-path tensorboard/data/server/Cargo.toml - name: 'Check cargo-raze freshness' run: | rm -rf third_party/rust/ - (cd tensorboard/data/server/ && cargo generate-lockfile && cargo raze) + (cd tensorboard/data/server/ && cargo fetch && cargo raze) git add . git diff --staged --exit-code - lint-docs: - runs-on: ubuntu-16.04 - steps: - - uses: actions/checkout@v1 - - uses: actions/setup-python@v1 - with: - python-version: '3.6' - architecture: 'x64' - - name: 'Install yamllint' - run: | - python -m pip install -U pip - pip install yamllint -c ./tensorboard/pip_package/requirements_dev.txt - - run: pip freeze --all - - name: 'Lint YAML for gotchas with yamllint' - # Use '# yamllint disable-line rule:foo' to suppress. - run: yamllint -c docs/.yamllint docs docs/.yamllint - - name: 'Install the TensorFlow docs notebook tools' - run: | - nbfmt_version="174c9a5c1cc51a3af1de98d84824c811ecd49029" - python3 -m pip install -U git+https://github.com/tensorflow/docs@${nbfmt_version} - - name: 'Use nbfmt to check Colab notebooks for formatting' - run: git ls-files -z '*.ipynb' | xargs -0 python3 -m tensorflow_docs.tools.nbfmt --test - lint-frontend: runs-on: ubuntu-16.04 steps: @@ -152,7 +214,7 @@ jobs: - run: | ! git grep -E '"@npm//d3"|"@npm//@types/d3"' 'tensorboard/webapp/**/*BUILD' ':!tensorboard/webapp/third_party/**' - lint-build: + lint-misc: # build, protos, etc. runs-on: ubuntu-16.04 steps: - uses: actions/checkout@v1 @@ -179,11 +241,6 @@ jobs: # https://github.com/bazelbuild/buildtools/blob/master/buildozer/README.md#error-code run: | buildozer '//tensorboard/...:%licenses' remove_comment && false || test $? = 3 - - lint-proto: - runs-on: ubuntu-16.04 - steps: - - uses: actions/checkout@v1 - name: clang-format lint uses: DoozyX/clang-format-lint-action@v0.5 with: @@ -192,11 +249,6 @@ jobs: exclude: ./tensorboard/compat/proto extensions: 'proto' clangFormatVersion: 9 - - check-misc: - runs-on: ubuntu-16.04 - steps: - - uses: actions/checkout@v1 - run: ./tensorboard/tools/do_not_submit_test.sh - run: ./tensorboard/tools/license_test.sh - run: ./tensorboard/tools/whitespace_hygiene_test.py diff --git a/.travis.yml b/.travis.yml deleted file mode 100644 index 1131967b47..0000000000 --- a/.travis.yml +++ /dev/null @@ -1,115 +0,0 @@ -dist: xenial -language: python -python: - - "3.6" - -branches: - only: - - master - - /^\d+\.\d+(\.\d+)?(-\S*)?$/ - -# Update the `nvm use` stanza below when updating this. -node_js: - - "11" - -env: - # Keep this Bazel version in sync with the `versions.check` directive - # near the top of our WORKSPACE file. - # - # Grab the BAZEL_SHA256SUM from the Bazel releases page; e.g.: - # bazel-0.20.0-linux-x86_64.sha256 - global: - - BAZEL=2.1.0 - - BAZEL_SHA256SUM=e13581d44faad6ac807dd917e682fef20359d26728166ac35dadd8ee653a580d - matrix: - # HACK: do not try the 2.4.0rc1 because Travis CI cannot reuse any caches. - # - TF_VERSION_ID=tensorflow==2.4.0rc1 - - TF_VERSION_ID= # Do not install TensorFlow in this case - -cache: - # Don't cache the Pip directory. We pull in a new `tf-nightly` wheel - # every day, and Pip caches are never evicted, so this quickly bloats - # to many gigabytes and adds minutes to the CI time. - pip: false - # Cache directories for Bazel. See ci/bazelrc for details. - directories: - - $HOME/.cache/tb-bazel-repo - - $HOME/.cache/tb-bazel-disk - -# Each bullet point is displayed in the Travis log as one collapsed line, which -# indicates how long it took. Travis will check the return code at the end. We -# can't use `set -e` in the YAML file since it might impact Travis internals. -# If inline scripts get too long, Travis surprisingly prints them twice. - -before_install: - - elapsed() { TZ=UTC printf "Time %(%T)T %s\n" "$SECONDS" "$1"; } - - elapsed "before_install" - - ci/download_bazel.sh "${BAZEL}" "${BAZEL_SHA256SUM}" ~/bazel - - sudo mv ~/bazel /usr/local/bin/bazel - - cp ci/bazelrc ~/.bazelrc - - elapsed "before_install (done)" - -install: - - elapsed "install" - - "PY3=\"$(python -c 'if __import__(\"sys\").version_info[0] > 2: print(1)')\"" - # Older versions of Pip sometimes resolve specifiers like `tf-nightly` - # to versions other than the most recent(!). - - pip install -U pip - # Uninstall older Travis numpy to avoid upgrade-in-place issues. - - pip uninstall -y numpy - - | - pip install \ - -r tensorboard/pip_package/requirements.txt \ - -r tensorboard/pip_package/requirements_dev.txt \ - ; - # Keep the node version in sync with node_js key above. - - nvm use v11 - # HACK: intentionally test just 2.4.0rc1, since the notf version was tested - # and historically passed in: - # https://travis-ci.org/github/tensorflow/tensorboard/builds/742878781 - - export TF_VERSION_ID=tensorflow==2.4.0rc1 - - | - # Install TensorFlow if requested - if [ -n "${TF_VERSION_ID}" ]; then - pip install -I "${TF_VERSION_ID}" - fi - # Workaround for https://github.com/travis-ci/travis-ci/issues/7940 - - sudo rm -f /etc/boto.cfg - - pip freeze # print installed distributions, for debugging purposes - - elapsed "install (done)" - -before_script: - # Note: Lint checks happen on GitHub Actions; see .github/workflows/ci.yml. - - elapsed "before_script" - - | - # Specify subset of tests to run depending on TF installation config. - # We condition the value of --test_tag_filters so that we can run the - # bazel test command unconditionally which produces nicer log output. - if [ -z "${TF_VERSION_ID}" ]; then - test_tag_filters=support_notf - else - test_tag_filters= - fi - - elapsed "before_script (done)" - -# Commands in this section should only fail if it's our fault. Travis will -# categorize them as 'failed', rather than 'error' for other sections. -script: - - elapsed "script" - # Note: bazel test implies fetch+build, but this gives us timing. - - elapsed && bazel fetch //tensorboard/... - - elapsed && bazel build //tensorboard/... - - elapsed && bazel test //tensorboard/... --test_tag_filters="${test_tag_filters}" - - elapsed && bazel run //tensorboard/pip_package:test_pip_package -- --tf-version "${TF_VERSION_ID}" - # Run manual S3 test - - elapsed && bazel test //tensorboard/compat/tensorflow_stub:gfile_s3_test - - elapsed && bazel test //tensorboard/summary/writer:event_file_writer_s3_test - - elapsed "script (done)" - -after_script: - # Bazel launches daemons unless --batch is used. - - elapsed "after_script" - - bazel shutdown - -notifications: - email: false diff --git a/ci/bazelrc b/ci/bazelrc index 10435bae8a..4bda9653d6 100644 --- a/ci/bazelrc +++ b/ci/bazelrc @@ -1,5 +1,6 @@ -# Limit resources since Travis Trusty GCE VMs have 2 cores and 7.5 GB RAM. -build --local_resources=4000,2,1.0 +# Limit resources since GitHub Actions VMs have 2 cores and 7 GB RAM. +build --local_cpu_resources=2 +build --local_ram_resources=4000 build --worker_max_instances=2 # Ensure sandboxing is on to increase hermeticity. @@ -17,18 +18,17 @@ build --worker_sandboxing # https://github.com/bazelbuild/bazel/issues/7026 (future of action_env) build --action_env=PATH -# Set up caching on local disk so incremental builds are faster. -# See https://bazel.build/designs/2016/09/30/repository-cache.html -build --repository_cache=~/.cache/tb-bazel-repo -fetch --repository_cache=~/.cache/tb-bazel-repo -query --repository_cache=~/.cache/tb-bazel-repo -# See https://docs.bazel.build/versions/master/remote-caching.html#disk-cache -build --disk_cache=~/.cache/tb-bazel-disk - -# Log more information to help with debugging, and disable curses output which -# just adds more clutter to the log. (Travis spoofs an interactive terminal.) -common --curses=no +# Log more information to help with debugging. build --verbose_failures build --worker_verbose test --test_output=errors test --test_verbose_timeout_warnings + +# Enable remote caching. This cache is publicly readable, but only writable on +# trusted commits (e.g., commits to master). We don't write the actual test +# statuses (or read them if they happen to be there) because not all tests are +# actually hermetic, and they run pretty quickly, anyway. +common --remote_cache=https://storage.googleapis.com/tensorboard-build-cache +common --remote_upload_local_results=false +test --remote_upload_local_results=false +test --cache_test_results=false diff --git a/tensorboard/pip_package/test_pip_package.sh b/tensorboard/pip_package/test_pip_package.sh index 738d5cd4dd..1595bcd860 100755 --- a/tensorboard/pip_package/test_pip_package.sh +++ b/tensorboard/pip_package/test_pip_package.sh @@ -24,7 +24,7 @@ Test pre-built TensorBoard Pip packages. Options: --tf-version VERSION: Test against the provided version of TensorFlow, given as a Pip package specifier like "tensorflow==2.0.0a0" or - "tf-nightly". If empty, will test without installing TensorFlow. + "tf-nightly". If empty or "notf", will test without installing TensorFlow. Defaults to "tf-nightly". EOF } @@ -104,6 +104,9 @@ smoke() ( smoke_venv="${virtualenvs_root}/venv-${smoke_python}/" set +x printf '\n\n%70s\n' '' | tr ' ' '=' + if [ "${tf_version}" = notf ]; then + tf_version= + fi if [ -z "${tf_version}" ]; then echo "Smoke testing with ${smoke_python} and no tensorflow..." else From 21b4035aff419598d647aca1ecbb61e94016cad0 Mon Sep 17 00:00:00 2001 From: Stephan Lee Date: Wed, 13 Jan 2021 14:54:54 -0800 Subject: [PATCH 2/3] ng: support path_prefix at router level (#4423) Problem: TensorBoard has a flag, path_prefix, which serves frontend and the data endpoints at your specified, if the flag is passed, pathname. While exact use case slips my mind, it is certainly used by dependents. Now, the problem is that new Angular based TensorBoard has a custom router for in-app navigation (we may add new routes for experiment list and others) it is unaware of the path prefix. Router works by matching set of predefined routes to current pathname and finds a match. However, the configuration is unaware (rightfully) of the path prefix and it results in zero match, which in turn cause navigation to the default path, "/". This totally breaks the assumption of the path prefix as all the request from the frontend will be sent to "/data/..." as opposed to "[path_prefix]/data/..." Remediation: Previously, there was no reason to propagate the prefix information to the frontend. Application only did hash changes and never required a navigation. With this change, we now: - server side render `` tag that depicts the relative path to the root of the application. - bootstrap Angular app from the relative root path and uses that information for matching routes and navigation --- tensorboard/plugins/core/core_plugin.py | 37 +++- tensorboard/plugins/core/core_plugin_test.py | 77 +++++++- tensorboard/webapp/app_routing/BUILD | 14 ++ tensorboard/webapp/app_routing/app_root.ts | 68 +++++++ .../webapp/app_routing/app_root_test.ts | 99 ++++++++++ .../webapp/app_routing/app_routing_module.ts | 3 +- tensorboard/webapp/app_routing/effects/BUILD | 2 + .../effects/app_routing_effects.ts | 46 ++++- .../effects/app_routing_effects_test.ts | 182 ++++++++++++++++-- tensorboard/webapp/app_routing/location.ts | 6 + .../webapp/app_routing/location_test.ts | 9 + tensorboard/webapp/app_routing/views/BUILD | 2 + .../views/app_routing_view_module.ts | 2 + .../views/router_link_directive_container.ts | 21 +- .../app_routing/views/router_link_test.ts | 37 +++- 15 files changed, 571 insertions(+), 34 deletions(-) create mode 100644 tensorboard/webapp/app_routing/app_root.ts create mode 100644 tensorboard/webapp/app_routing/app_root_test.ts diff --git a/tensorboard/plugins/core/core_plugin.py b/tensorboard/plugins/core/core_plugin.py index b1685f6c3a..085fbf86aa 100644 --- a/tensorboard/plugins/core/core_plugin.py +++ b/tensorboard/plugins/core/core_plugin.py @@ -21,6 +21,7 @@ import functools import gzip import mimetypes +import posixpath import zipfile import six @@ -58,6 +59,7 @@ def __init__(self, context): logdir_spec = context.flags.logdir_spec if context.flags else "" self._logdir = context.logdir or logdir_spec self._window_title = context.window_title + self._path_prefix = context.flags.path_prefix if context.flags else None self._assets_zip_provider = context.assets_zip_provider self._data_provider = context.data_provider @@ -91,7 +93,15 @@ def get_resource_apps(self): with self._assets_zip_provider() as fp: with zipfile.ZipFile(fp) as zip_: for path in zip_.namelist(): - gzipped_asset_bytes = _gzip(zip_.read(path)) + content = zip_.read(path) + # Opt out of gzipping index.html + if path == "index.html": + apps["/" + path] = functools.partial( + self._serve_index, content + ) + continue + + gzipped_asset_bytes = _gzip(content) wsgi_app = functools.partial( self._serve_asset, path, gzipped_asset_bytes ) @@ -115,6 +125,31 @@ def _serve_asset(self, path, gzipped_asset_bytes, request): request, gzipped_asset_bytes, mimetype, content_encoding="gzip" ) + @wrappers.Request.application + def _serve_index(self, index_asset_bytes, request): + """Serves index.html content. + + Note that we opt out of gzipping index.html to write preamble before the + resource content. This inflates the resource size from 2x kiB to 1xx + kiB, but we require an ability to flush preamble with the HTML content. + """ + relpath = ( + posixpath.relpath(self._path_prefix, request.script_root) + if self._path_prefix + else "." + ) + meta_header = ( + '' + % relpath + ) + content = meta_header.encode("utf-8") + index_asset_bytes + # By passing content_encoding, disallow gzipping. Bloats the content + # from ~25 kiB to ~120 kiB but reduces CPU usage and avoid 3ms worth of + # gzipping. + return http_util.Respond( + request, content, "text/html", content_encoding="identity" + ) + @wrappers.Request.application def _serve_environment(self, request): """Serve a JSON object containing some base properties used by the diff --git a/tensorboard/plugins/core/core_plugin_test.py b/tensorboard/plugins/core/core_plugin_test.py index 510a57b655..125468b087 100644 --- a/tensorboard/plugins/core/core_plugin_test.py +++ b/tensorboard/plugins/core/core_plugin_test.py @@ -170,7 +170,11 @@ def testIndex_returnsActualHtml(self): self.assertEqual(200, response.status_code) self.assertStartsWith(response.headers.get("Content-Type"), "text/html") html = response.get_data() - self.assertEqual(html, FAKE_INDEX_HTML) + self.assertEqual( + html, + b'' + + FAKE_INDEX_HTML, + ) def testDataPaths_disableAllCaching(self): """Test the format of the /data/runs endpoint.""" @@ -378,6 +382,77 @@ def FirstEventTimestamp_stub(run_name): ) +class CorePluginPathPrefixTest(tf.test.TestCase): + def _send_request(self, path_prefix, pathname): + multiplexer = event_multiplexer.EventMultiplexer() + logdir = self.get_temp_dir() + provider = data_provider.MultiplexerDataProvider(multiplexer, logdir) + context = base_plugin.TBContext( + assets_zip_provider=get_test_assets_zip_provider(), + logdir=logdir, + data_provider=provider, + window_title="", + flags=FakeFlags(path_prefix=path_prefix), + ) + plugin = core_plugin.CorePlugin(context) + app = application.TensorBoardWSGI([plugin], path_prefix=path_prefix) + server = werkzeug_test.Client(app, wrappers.BaseResponse) + return server.get(pathname) + + def _assert_index(self, response, expected_tb_relative_root): + self.assertEqual(200, response.status_code) + self.assertStartsWith(response.headers.get("Content-Type"), "text/html") + html = response.get_data() + + expected_meta = ( + '' + % expected_tb_relative_root + ).encode() + self.assertEqual( + html, + expected_meta + FAKE_INDEX_HTML, + ) + + def testIndex_no_path_prefix(self): + self._assert_index(self._send_request("", "/"), "./") + self._assert_index(self._send_request("", "/index.html"), "./") + + def testIndex_path_prefix_foo(self): + self._assert_index(self._send_request("/foo", "/foo/"), "./") + self._assert_index(self._send_request("/foo", "/foo/index.html"), "./") + + def testIndex_path_prefix_foo_exp_route(self): + self._assert_index( + self._send_request("/foo", "/foo/experiment/123/"), "../../" + ) + + def testIndex_path_prefix_foo_incorrect_route(self): + self.assertEqual( + 404, (self._send_request("/foo", "/foo/meow/").status_code) + ) + self.assertEqual(404, (self._send_request("/foo", "/").status_code)) + self.assertEqual( + 404, (self._send_request("/foo", "/index.html").status_code) + ) + + # Missing trailing "/" causes redirection. + self.assertEqual(301, (self._send_request("/foo", "/foo").status_code)) + self.assertEqual( + 301, (self._send_request("/foo", "/foo/experiment/123").status_code) + ) + + def testIndex_path_prefix_foo_bar(self): + self._assert_index(self._send_request("/foo/bar", "/foo/bar/"), "./") + self._assert_index( + self._send_request("/foo/bar", "/foo/bar/index.html"), "./" + ) + + def testIndex_path_prefix_foo_bar_exp_route(self): + self._assert_index( + self._send_request("/foo/bar", "/foo/bar/experiment/123/"), "../../" + ) + + def get_test_assets_zip_provider(): memfile = six.BytesIO() with zipfile.ZipFile( diff --git a/tensorboard/webapp/app_routing/BUILD b/tensorboard/webapp/app_routing/BUILD index 47e35698b8..00336184b8 100644 --- a/tensorboard/webapp/app_routing/BUILD +++ b/tensorboard/webapp/app_routing/BUILD @@ -10,6 +10,7 @@ tf_ng_module( "app_routing_module.ts", ], deps = [ + ":app_root", ":location", ":programmatical_navigation_module", ":route_registry", @@ -36,6 +37,17 @@ tf_ng_module( ], ) +tf_ng_module( + name = "app_root", + srcs = [ + "app_root.ts", + ], + deps = [ + ":location", + "@npm//@angular/core", + ], +) + tf_ng_module( name = "route_registry", srcs = [ @@ -152,6 +164,7 @@ tf_ng_module( name = "app_routing_test", testonly = True, srcs = [ + "app_root_test.ts", "internal_utils_test.ts", "location_test.ts", "programmatical_navigation_module_test.ts", @@ -159,6 +172,7 @@ tf_ng_module( "route_registry_module_test.ts", ], deps = [ + ":app_root", ":internal_utils", ":location", ":programmatical_navigation_module", diff --git a/tensorboard/webapp/app_routing/app_root.ts b/tensorboard/webapp/app_routing/app_root.ts new file mode 100644 index 0000000000..be97930466 --- /dev/null +++ b/tensorboard/webapp/app_routing/app_root.ts @@ -0,0 +1,68 @@ +/* Copyright 2020 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ +import {Injectable} from '@angular/core'; + +import {Location} from './location'; + +@Injectable() +export class AppRootProvider { + protected appRoot: string; + + constructor(location: Location) { + this.appRoot = this.getAppRootFromMetaElement(location); + } + + /** + * appRoot path starts with `/` and always end with `/`. + */ + private getAppRootFromMetaElement(location: Location): string { + const metaEl = document.querySelector('head meta[name="tb-relative-root"]'); + + if (!metaEl) return '/'; + const {pathname} = new URL( + (metaEl as HTMLMetaElement).content, + location.getHref() + ); + return pathname.replace(/\/*$/, '/'); + } + + /** + * Returns actual full pathname that includes path prefix of the application. + */ + getAbsPathnameWithAppRoot(absPathname: string): string { + // appRoot has trailing '/'. Remove one so we don't have "//". + return this.appRoot.slice(0, -1) + absPathname; + } + + getAppRootlessPathname(absPathname: string) { + if (absPathname.startsWith(this.appRoot)) { + // appRoot ends with "/" and we need the trimmed pathname to start with "/" since + // routes are defined with starting "/". + return '/' + absPathname.slice(this.appRoot.length); + } + return absPathname; + } +} + +@Injectable() +export class TestableAppRootProvider extends AppRootProvider { + getAppRoot(): string { + return this.appRoot; + } + + setAppRoot(appRoot: string) { + this.appRoot = appRoot; + } +} diff --git a/tensorboard/webapp/app_routing/app_root_test.ts b/tensorboard/webapp/app_routing/app_root_test.ts new file mode 100644 index 0000000000..831c29b355 --- /dev/null +++ b/tensorboard/webapp/app_routing/app_root_test.ts @@ -0,0 +1,99 @@ +/* Copyright 2020 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ +import {TestBed} from '@angular/core/testing'; + +import {AppRootProvider, TestableAppRootProvider} from './app_root'; +import {Location} from './location'; + +describe('app root', () => { + let getHrefSpy: jasmine.Spy; + + beforeEach(async () => { + getHrefSpy = jasmine.createSpy(); + await TestBed.configureTestingModule({ + providers: [ + Location, + {provide: AppRootProvider, useClass: TestableAppRootProvider}, + ], + }).compileComponents(); + + const location = TestBed.inject(Location); + getHrefSpy = spyOn(location, 'getHref').and.returnValue('https://tb.dev/'); + }); + + function setUp(href: string, content: string): TestableAppRootProvider { + getHrefSpy.and.returnValue(href); + const meta = document.createElement('meta'); + meta.name = 'tb-relative-root'; + meta.content = content; + document.head.appendChild(meta); + const appRoot = TestBed.inject(AppRootProvider) as TestableAppRootProvider; + document.head.removeChild(meta); + return appRoot; + } + + [ + {href: 'https://tb.dev/', content: './', expectedAppRoot: '/'}, + {href: 'https://tb.dev/index.html', content: './', expectedAppRoot: '/'}, + { + href: 'https://tb.dev/foo/bar/experiment/1/', + content: '../../', + expectedAppRoot: '/foo/bar/', + }, + // wrong relative content but we handle it correctly. + {href: 'https://tb.dev/', content: '../../', expectedAppRoot: '/'}, + {href: 'https://tb.dev/', content: './/', expectedAppRoot: '/'}, + { + href: 'https://tb.dev/experiment/1/', + content: '../..///', + expectedAppRoot: '/', + }, + ].forEach(({content, href, expectedAppRoot}) => { + describe('appRoot parsing', () => { + it(`returns an absolute path from : ${href} and ${content}`, () => { + expect(setUp(href, content).getAppRoot()).toBe(expectedAppRoot); + }); + }); + }); + + describe('#getAbsPathnameWithAppRoot', () => { + it('returns pathname with appRoot', () => { + expect( + setUp( + 'https://tb.dev/foo/bar/experiment/1/', + '../../' + ).getAbsPathnameWithAppRoot('/cool/test') + ).toBe(`/foo/bar/cool/test`); + }); + }); + + describe('#getAppRootlessPathname', () => { + it('returns a path without app root', () => { + const provider = setUp('https://tb.dev/foo/bar/experiment/1/', '../../'); + expect(provider.getAppRootlessPathname('/foo/bar/')).toBe('/'); + expect(provider.getAppRootlessPathname('/foo/bar/baz')).toBe('/baz'); + }); + + it('does not strip if pathname does not start with appRoot', () => { + const provider = setUp('https://tb.dev/foo/bar/experiment/1/', '../../'); + // misses trailing "/" to exactly match the appRoot. + expect(provider.getAppRootlessPathname('/foo/bar')).toBe('/foo/bar'); + expect(provider.getAppRootlessPathname('/bar')).toBe('/bar'); + expect(provider.getAppRootlessPathname('/fan/foo/bar')).toBe( + '/fan/foo/bar' + ); + }); + }); +}); diff --git a/tensorboard/webapp/app_routing/app_routing_module.ts b/tensorboard/webapp/app_routing/app_routing_module.ts index 670204a6e5..969890a5fb 100644 --- a/tensorboard/webapp/app_routing/app_routing_module.ts +++ b/tensorboard/webapp/app_routing/app_routing_module.ts @@ -16,6 +16,7 @@ import {NgModule} from '@angular/core'; import {EffectsModule} from '@ngrx/effects'; import {StoreModule} from '@ngrx/store'; +import {AppRootProvider} from './app_root'; import {AppRoutingEffects} from './effects'; import {LocationModule} from './location_module'; import {ProgrammaticalNavigationModule} from './programmatical_navigation_module'; @@ -28,6 +29,6 @@ import {APP_ROUTING_FEATURE_KEY} from './store/app_routing_types'; EffectsModule.forFeature([AppRoutingEffects]), LocationModule, ], - providers: [ProgrammaticalNavigationModule], + providers: [ProgrammaticalNavigationModule, AppRootProvider], }) export class AppRoutingModule {} diff --git a/tensorboard/webapp/app_routing/effects/BUILD b/tensorboard/webapp/app_routing/effects/BUILD index 0e5588d1db..99b5bf5982 100644 --- a/tensorboard/webapp/app_routing/effects/BUILD +++ b/tensorboard/webapp/app_routing/effects/BUILD @@ -12,6 +12,7 @@ tf_ng_module( ], deps = [ "//tensorboard/webapp:app_state", + "//tensorboard/webapp/app_routing:app_root", "//tensorboard/webapp/app_routing:internal_utils", "//tensorboard/webapp/app_routing:location", "//tensorboard/webapp/app_routing:programmatical_navigation_module", @@ -36,6 +37,7 @@ tf_ng_module( "//tensorboard/webapp:app_state", "//tensorboard/webapp/angular:expect_angular_core_testing", "//tensorboard/webapp/angular:expect_ngrx_store_testing", + "//tensorboard/webapp/app_routing:app_root", "//tensorboard/webapp/app_routing:location", "//tensorboard/webapp/app_routing:programmatical_navigation_module", "//tensorboard/webapp/app_routing:route_registry", diff --git a/tensorboard/webapp/app_routing/effects/app_routing_effects.ts b/tensorboard/webapp/app_routing/effects/app_routing_effects.ts index 831f8bc6ec..5e04408632 100644 --- a/tensorboard/webapp/app_routing/effects/app_routing_effects.ts +++ b/tensorboard/webapp/app_routing/effects/app_routing_effects.ts @@ -12,7 +12,7 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ -import {Injectable} from '@angular/core'; +import {Inject, Injectable} from '@angular/core'; import {Actions, createEffect, ofType} from '@ngrx/effects'; import {Action, createAction, Store} from '@ngrx/store'; import {merge, Observable, of} from 'rxjs'; @@ -33,6 +33,7 @@ import { navigationRequested, stateRehydratedFromUrl, } from '../actions'; +import {AppRootProvider} from '../app_root'; import {areRoutesEqual, getRouteId} from '../internal_utils'; import {Location} from '../location'; import {ProgrammaticalNavigationModule} from '../programmatical_navigation_module'; @@ -60,13 +61,20 @@ export class AppRoutingEffects { private readonly store: Store, private readonly location: Location, registry: RouteRegistryModule, - private readonly programmaticalNavModule: ProgrammaticalNavigationModule + private readonly programmaticalNavModule: ProgrammaticalNavigationModule, + private readonly appRootProvider: AppRootProvider ) { this.routeConfigs = registry.getRouteConfigs(); } private readonly onNavigationRequested$ = this.actions$.pipe( - ofType(navigationRequested) + ofType(navigationRequested), + map((navigation) => { + const resolvedPathname = navigation.pathname.startsWith('/') + ? this.appRootProvider.getAbsPathnameWithAppRoot(navigation.pathname) + : this.location.getResolvedPath(navigation.pathname); + return {...navigation, pathname: resolvedPathname}; + }) ); private readonly onInit$: Observable = this.actions$ @@ -83,19 +91,35 @@ export class AppRoutingEffects { }) ); + /** + * Input observable must have absolute pathname with, when appRoot is present, + * appRoot prefixed (e.g., window.location.pathname). + */ private readonly userInitNavRoute$ = merge( this.onNavigationRequested$, this.onInit$, this.location.onPopState().pipe( map((navigation) => { - return {...navigation, browserInitiated: true}; + return { + pathname: navigation.pathname, + replaceState: navigation.replaceState, + browserInitiated: true, + }; }) ) ).pipe( map((navigation) => { + // Expect to have absolute navigation here. + if (!navigation.pathname.startsWith('/')) { + throw new Error( + `[App routing] pathname must start with '/'. Got: ${navigation.pathname}` + ); + } return { ...navigation, - pathname: this.location.getResolvedPath(navigation.pathname), + pathname: this.appRootProvider.getAppRootlessPathname( + navigation.pathname + ), }; }), map((navigationWithAbsolutePath) => { @@ -254,18 +278,24 @@ export class AppRoutingEffects { }), filter(({route}) => { return !areRoutesEqual(route, { - pathname: this.location.getPath(), + pathname: this.appRootProvider.getAppRootlessPathname( + this.location.getPath() + ), queryParams: this.location.getSearch(), }); }), tap(({preserveHash, route}) => { if (route.navigationOptions.replaceState) { this.location.replaceState( - this.location.getFullPathFromRouteOrNav(route, preserveHash) + this.appRootProvider.getAbsPathnameWithAppRoot( + this.location.getFullPathFromRouteOrNav(route, preserveHash) + ) ); } else { this.location.pushState( - this.location.getFullPathFromRouteOrNav(route, preserveHash) + this.appRootProvider.getAbsPathnameWithAppRoot( + this.location.getFullPathFromRouteOrNav(route, preserveHash) + ) ); } }) diff --git a/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts b/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts index fffde9a441..7a2941b49d 100644 --- a/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts +++ b/tensorboard/webapp/app_routing/effects/app_routing_effects_test.ts @@ -22,6 +22,8 @@ import {of, ReplaySubject} from 'rxjs'; import {State} from '../../app_state'; import * as actions from '../actions'; +import {navigationRequested} from '../actions'; +import {AppRootProvider, TestableAppRootProvider} from '../app_root'; import {Location} from '../location'; import { NavigateToExperiments, @@ -46,6 +48,10 @@ describe('app_routing_effects', () => { let location: Location; let actualActions: Action[]; let onPopStateSubject: ReplaySubject; + let pushStateSpy: jasmine.Spy; + let getHashSpy: jasmine.Spy; + let getPathSpy: jasmine.Spy; + let getSearchSpy: jasmine.Spy; let serializeStateToQueryParamsSpy: jasmine.Spy; let deserializeQueryParamsSpy: jasmine.Spy; @@ -67,6 +73,7 @@ describe('app_routing_effects', () => { routeKind: RouteKind.EXPERIMENTS, path: '/experiments', ngComponent: TestableComponent, + defaultRoute: true, }, { routeKind: RouteKind.COMPARE_EXPERIMENT, @@ -104,6 +111,10 @@ describe('app_routing_effects', () => { AppRoutingEffects, provideMockStore(), provideLocationTesting(), + { + provide: AppRootProvider, + useClass: TestableAppRootProvider, + }, ], }).compileComponents(); @@ -114,17 +125,19 @@ describe('app_routing_effects', () => { location = TestBed.inject(TestableLocation) as Location; onPopStateSubject = new ReplaySubject(1); spyOn(location, 'onPopState').and.returnValue(onPopStateSubject); - store.overrideSelector(getActiveRoute, null); + pushStateSpy = spyOn(location, 'pushState'); + getHashSpy = spyOn(location, 'getHash').and.returnValue(''); + getPathSpy = spyOn(location, 'getPath').and.returnValue(''); + getSearchSpy = spyOn(location, 'getSearch').and.returnValue([]); - effects = TestBed.inject(AppRoutingEffects); + store.overrideSelector(getActiveRoute, null); }); describe('fireNavigatedIfValidRoute$', () => { - let getPathSpy: jasmine.Spy; - let getSearchSpy: jasmine.Spy; let actualActions: Action[]; beforeEach(() => { + effects = TestBed.inject(AppRoutingEffects); actualActions = []; spyOn(store, 'dispatch').and.callFake((action: Action) => { @@ -133,9 +146,6 @@ describe('app_routing_effects', () => { effects.fireNavigatedIfValidRoute$.subscribe((action) => { actualActions.push(action); }); - - getPathSpy = spyOn(location, 'getPath'); - getSearchSpy = spyOn(location, 'getSearch'); }); afterEach(fakeAsync(() => { @@ -534,19 +544,11 @@ describe('app_routing_effects', () => { describe('changeBrowserUrl$', () => { let replaceStateSpy: jasmine.Spy; - let pushStateSpy: jasmine.Spy; - let getHashSpy: jasmine.Spy; - let getPathSpy: jasmine.Spy; - let getSearchSpy: jasmine.Spy; beforeEach(() => { + effects = TestBed.inject(AppRoutingEffects); effects.changeBrowserUrl$.subscribe(() => {}); - replaceStateSpy = spyOn(location, 'replaceState'); - pushStateSpy = spyOn(location, 'pushState'); - getHashSpy = spyOn(location, 'getHash'); - getPathSpy = spyOn(location, 'getPath'); - getSearchSpy = spyOn(location, 'getSearch'); }); it('noops if the new route matches current URL', () => { @@ -724,4 +726,152 @@ describe('app_routing_effects', () => { expect(replaceStateSpy).toHaveBeenCalledWith('/experiment'); }); }); + + describe('path_prefix support', () => { + function setAppRootAndSubscribe(appRoot: string) { + const provider = TestBed.inject( + AppRootProvider + ) as TestableAppRootProvider; + provider.setAppRoot(appRoot); + + effects = TestBed.inject(AppRoutingEffects); + const dispatchSpy = spyOn(store, 'dispatch'); + effects.fireNavigatedIfValidRoute$.subscribe((action) => { + actualActions.push(action); + }); + + actualActions = []; + dispatchSpy.and.callFake((action: Action) => { + actualActions.push(action); + }); + + effects.changeBrowserUrl$.subscribe(() => {}); + } + + it('navigates to default route if popstated to path without prefix', fakeAsync(() => { + setAppRootAndSubscribe('/foo/bar/'); + + onPopStateSubject.next({ + pathname: '/meow', + }); + + expect(actualActions).toEqual([ + actions.navigating({ + after: buildRoute({ + routeKind: RouteKind.EXPERIMENTS, + params: {}, + pathname: '/experiments', + queryParams: [], + navigationOptions: { + replaceState: false, + }, + }), + }), + ]); + + tick(); + })); + + it('navigates to a matching route if popstated to path with prefix', fakeAsync(() => { + setAppRootAndSubscribe('/foo/bar/'); + + onPopStateSubject.next({ + pathname: '/foo/bar/experiment/123', + }); + + expect(actualActions).toEqual([ + actions.navigating({ + after: buildRoute({ + routeKind: RouteKind.EXPERIMENT, + params: {experimentId: '123'}, + pathname: '/experiment/123', + queryParams: [], + navigationOptions: { + replaceState: false, + }, + }), + }), + ]); + + tick(); + })); + + it('navigates with appRoot aware path when navRequest with absPath', fakeAsync(() => { + setAppRootAndSubscribe('/foo/bar/'); + + // Do note that this path name does not contain the appRoot. + action.next(navigationRequested({pathname: '/experiment/123'})); + + expect(actualActions).toEqual([ + actions.navigating({ + after: buildRoute({ + routeKind: RouteKind.EXPERIMENT, + params: {experimentId: '123'}, + pathname: '/experiment/123', + queryParams: [], + navigationOptions: { + replaceState: false, + }, + }), + }), + ]); + + tick(); + })); + + it('navigates with appRoot aware path when navRequest with relPath', fakeAsync(() => { + setAppRootAndSubscribe('/foo/bar/'); + + spyOn(location, 'getResolvedPath') + .withArgs('../experiment/123') + .and.returnValue('/foo/bar/experiment/123'); + + // Do note that this path name does not contain the appRoot. + action.next(navigationRequested({pathname: '../experiment/123'})); + + expect(actualActions).toEqual([ + actions.navigating({ + after: buildRoute({ + routeKind: RouteKind.EXPERIMENT, + params: {experimentId: '123'}, + pathname: '/experiment/123', + queryParams: [], + navigationOptions: { + replaceState: false, + }, + }), + }), + ]); + + tick(); + })); + + describe('change url', () => { + it('navigates to URL with path prefix prefixed', fakeAsync(() => { + setAppRootAndSubscribe('/foo/bar/baz/'); + const activeRoute = buildRoute({ + routeKind: RouteKind.EXPERIMENTS, + pathname: '/experiments', + queryParams: [], + navigationOptions: { + replaceState: false, + }, + }); + store.overrideSelector(getActiveRoute, activeRoute); + store.refreshState(); + getHashSpy.and.returnValue(''); + getPathSpy.and.returnValue(''); + getSearchSpy.and.returnValue([]); + + action.next( + actions.navigated({ + before: null, + after: activeRoute, + }) + ); + + expect(pushStateSpy).toHaveBeenCalledWith('/foo/bar/baz/experiments'); + })); + }); + }); }); diff --git a/tensorboard/webapp/app_routing/location.ts b/tensorboard/webapp/app_routing/location.ts index c4e8e70e30..db0cfd84b8 100644 --- a/tensorboard/webapp/app_routing/location.ts +++ b/tensorboard/webapp/app_routing/location.ts @@ -20,6 +20,8 @@ import {createURLSearchParamsFromSerializableQueryParams} from './internal_utils import {Navigation, Route, SerializableQueryParams} from './types'; export interface LocationInterface { + getHref(): string; + getSearch(): SerializableQueryParams; getHash(): string; @@ -54,6 +56,10 @@ function isNavigation( @Injectable() export class Location implements LocationInterface { + getHref(): string { + return utils.getHref(); + } + getSearch(): SerializableQueryParams { const searchParams = new URLSearchParams(window.location.search); const serializableSearchParams: SerializableQueryParams = []; diff --git a/tensorboard/webapp/app_routing/location_test.ts b/tensorboard/webapp/app_routing/location_test.ts index f5e71de026..f463028261 100644 --- a/tensorboard/webapp/app_routing/location_test.ts +++ b/tensorboard/webapp/app_routing/location_test.ts @@ -22,6 +22,15 @@ describe('location', () => { location = new Location(); }); + describe('#getHref', () => { + it('returns href', () => { + spyOn(TEST_ONLY.utils, 'getHref').and.returnValue( + 'https://t.b/is/cool/product' + ); + expect(location.getHref()).toBe('https://t.b/is/cool/product'); + }); + }); + describe('#getResolvedPath', () => { it('forms absolute path from current href', () => { spyOn(TEST_ONLY.utils, 'getHref').and.returnValue( diff --git a/tensorboard/webapp/app_routing/views/BUILD b/tensorboard/webapp/app_routing/views/BUILD index 3c27bce166..22e2339313 100644 --- a/tensorboard/webapp/app_routing/views/BUILD +++ b/tensorboard/webapp/app_routing/views/BUILD @@ -14,6 +14,7 @@ tf_ng_module( ], deps = [ "//tensorboard/webapp:app_state", + "//tensorboard/webapp/app_routing:app_root", "//tensorboard/webapp/app_routing:internal_utils", "//tensorboard/webapp/app_routing:location", "//tensorboard/webapp/app_routing:route_registry", @@ -41,6 +42,7 @@ tf_ts_library( "//tensorboard/webapp/angular:expect_angular_platform_browser_animations", "//tensorboard/webapp/angular:expect_angular_platform_browser_dynamic_testing", "//tensorboard/webapp/angular:expect_ngrx_store_testing", + "//tensorboard/webapp/app_routing:app_root", "//tensorboard/webapp/app_routing:location", "//tensorboard/webapp/app_routing:route_registry", "//tensorboard/webapp/app_routing:testing", diff --git a/tensorboard/webapp/app_routing/views/app_routing_view_module.ts b/tensorboard/webapp/app_routing/views/app_routing_view_module.ts index 2ce4bd2cc4..d12f25e7af 100644 --- a/tensorboard/webapp/app_routing/views/app_routing_view_module.ts +++ b/tensorboard/webapp/app_routing/views/app_routing_view_module.ts @@ -21,6 +21,7 @@ import {RouterLinkDirectiveContainer} from './router_link_directive_container'; import {RouterOutletComponent} from './router_outlet_component'; import {RouterOutletContainer} from './router_outlet_container'; import {LocationModule} from '../location_module'; +import {AppRootProvider} from '../app_root'; @NgModule({ imports: [CommonModule, LocationModule, RouteRegistryModule], @@ -30,5 +31,6 @@ import {LocationModule} from '../location_module'; RouterOutletComponent, RouterLinkDirectiveContainer, ], + providers: [AppRootProvider], }) export class AppRoutingViewModule {} diff --git a/tensorboard/webapp/app_routing/views/router_link_directive_container.ts b/tensorboard/webapp/app_routing/views/router_link_directive_container.ts index 2dab0d9fe9..7f13f95a45 100644 --- a/tensorboard/webapp/app_routing/views/router_link_directive_container.ts +++ b/tensorboard/webapp/app_routing/views/router_link_directive_container.ts @@ -12,11 +12,18 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ -import {Directive, HostBinding, HostListener, Input} from '@angular/core'; +import { + Directive, + HostBinding, + HostListener, + Inject, + Input, +} from '@angular/core'; import {Store} from '@ngrx/store'; import {State} from '../../app_state'; import {navigationRequested} from '../actions'; +import {AppRootProvider} from '../app_root'; import {Location} from '../location'; @Directive({selector: 'a[routerLink]'}) @@ -25,7 +32,8 @@ export class RouterLinkDirectiveContainer { constructor( private readonly store: Store, - private readonly location: Location + private readonly location: Location, + private readonly appRootProvider: AppRootProvider ) {} @HostListener('click', ['$event']) @@ -46,10 +54,11 @@ export class RouterLinkDirectiveContainer { @HostBinding('attr.href') get href() { if (!this.pathname) return null; - - return this.location.getFullPathFromRouteOrNav({ - pathname: this.pathname, - }); + return this.appRootProvider.getAbsPathnameWithAppRoot( + this.location.getFullPathFromRouteOrNav({ + pathname: this.pathname, + }) + ); } /** diff --git a/tensorboard/webapp/app_routing/views/router_link_test.ts b/tensorboard/webapp/app_routing/views/router_link_test.ts index 5f26b0561d..2547aa133f 100644 --- a/tensorboard/webapp/app_routing/views/router_link_test.ts +++ b/tensorboard/webapp/app_routing/views/router_link_test.ts @@ -22,6 +22,7 @@ import {MockStore, provideMockStore} from '@ngrx/store/testing'; import {State} from '../../app_state'; import {navigationRequested} from '../actions'; +import {AppRootProvider, TestableAppRootProvider} from '../app_root'; import {LocationModule} from '../location_module'; import {RouterLinkDirectiveContainer} from './router_link_directive_container'; @@ -36,17 +37,26 @@ class TestableComponent { describe('router_link', () => { let actualDispatches: Action[]; + let appRootProvider: TestableAppRootProvider; beforeEach(async () => { actualDispatches = []; + await TestBed.configureTestingModule({ imports: [LocationModule, NoopAnimationsModule], - providers: [provideMockStore()], + providers: [ + provideMockStore(), + {provide: AppRootProvider, useClass: TestableAppRootProvider}, + ], declarations: [RouterLinkDirectiveContainer, TestableComponent], schemas: [NO_ERRORS_SCHEMA], }).compileComponents(); const store = TestBed.inject>(Store) as MockStore; + appRootProvider = TestBed.inject( + AppRootProvider + ) as TestableAppRootProvider; + spyOn(store, 'dispatch').and.callFake((action: Action) => { actualDispatches.push(action); }); @@ -72,6 +82,18 @@ describe('router_link', () => { expect(anchorArr.attributes['href']).toBe('/foobar/baz/'); }); + it('renders the path as href with appRoot to support path_prefix', () => { + appRootProvider.setAppRoot('/qaz/quz/'); + const anchorStr = createComponentAndGetAnchorDebugElement('/foobar'); + expect(anchorStr.attributes['href']).toBe('/qaz/quz/foobar/'); + + const anchorArr = createComponentAndGetAnchorDebugElement([ + '/foobar', + 'baz', + ]); + expect(anchorArr.attributes['href']).toBe('/qaz/quz/foobar/baz/'); + }); + it('dispatches navigate when clicked on the anchor', () => { const link = createComponentAndGetAnchorDebugElement('/foobar'); const event = new MouseEvent('click'); @@ -84,6 +106,19 @@ describe('router_link', () => { ]); }); + it('dispatches programmatical navigation without appRoot', () => { + appRootProvider.setAppRoot('/qaz/quz/'); + const link = createComponentAndGetAnchorDebugElement('../foobar'); + const event = new MouseEvent('click'); + link.triggerEventHandler('click', event); + + expect(actualDispatches).toEqual([ + navigationRequested({ + pathname: '../foobar/', + }), + ]); + }); + it('supports relative path (..)', () => { const link = createComponentAndGetAnchorDebugElement('../foobar'); const event = new MouseEvent('click'); From 7abbe46e5eda6f5909e1b016f123b3a1648cff97 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Wed, 13 Jan 2021 17:37:25 -0800 Subject: [PATCH 3/3] Reformat `core_plugin_test` with Black 19.10b0 --- tensorboard/plugins/core/core_plugin_test.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tensorboard/plugins/core/core_plugin_test.py b/tensorboard/plugins/core/core_plugin_test.py index 125468b087..63e997b5a0 100644 --- a/tensorboard/plugins/core/core_plugin_test.py +++ b/tensorboard/plugins/core/core_plugin_test.py @@ -409,8 +409,7 @@ def _assert_index(self, response, expected_tb_relative_root): % expected_tb_relative_root ).encode() self.assertEqual( - html, - expected_meta + FAKE_INDEX_HTML, + html, expected_meta + FAKE_INDEX_HTML, ) def testIndex_no_path_prefix(self):