rusackas commented on code in PR #42511: URL: https://github.com/apache/superset/pull/42511#discussion_r3668332142
########## superset-frontend/spec/scripts/bundle-size-summary.test.js: ########## @@ -0,0 +1,107 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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. + */ +const fs = require('fs'); +const { + entrypointSizeByExt, + main, +} = require('../../scripts/bundle-size-summary'); + +function mockStats(entrypoints) { + jest + .spyOn(fs, 'readFileSync') + .mockReturnValue(JSON.stringify({ entrypoints })); +} + +function mockExit() { + return jest.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit called'); + }); +} + +afterEach(() => { + jest.restoreAllMocks(); +}); + +test('entrypointSizeByExt sums only assets matching the given extension', () => { + const entrypoint = { + assets: [ + { name: 'spa.entry.js', size: 100 }, + { name: 'spa.entry.js.map', size: 500 }, + { name: 'spa.entry.css', size: 20 }, + ], + }; + expect(entrypointSizeByExt(entrypoint, '.js')).toBe(100); + expect(entrypointSizeByExt(entrypoint, '.css')).toBe(20); +}); + +test('entrypointSizeByExt returns 0 when the entrypoint has no assets', () => { + expect(entrypointSizeByExt({}, '.js')).toBe(0); +}); + +test('main prints byte totals for every tracked entrypoint', () => { + mockStats({ + spa: { assets: [{ name: 'spa.js', size: 100 }] }, + embedded: { assets: [{ name: 'embedded.js', size: 50 }] }, + }); + const logSpy = jest.spyOn(console, 'log').mockImplementation(() => {}); + process.argv = ['node', 'bundle-size-summary.js', 'stats.json']; + + main(); + + const printed = JSON.parse(logSpy.mock.calls[0][0]); + expect(printed).toEqual([ + { name: 'spa entrypoint (JS)', unit: 'bytes', value: 100 }, + { name: 'spa entrypoint (CSS)', unit: 'bytes', value: 0 }, + { name: 'embedded entrypoint (JS)', unit: 'bytes', value: 50 }, + { name: 'embedded entrypoint (CSS)', unit: 'bytes', value: 0 }, + ]); +}); + +test('main exits with an error when a tracked entrypoint is missing from stats.json', () => { + mockStats({ spa: { assets: [] } }); + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + mockExit(); + process.argv = ['node', 'bundle-size-summary.js', 'stats.json']; + + expect(main).toThrow('process.exit called'); + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining('missing the "embedded" entrypoint'), + ); +}); + +test('main exits with an error when stats.json has no `entrypoints` key', () => { + jest.spyOn(fs, 'readFileSync').mockReturnValue(JSON.stringify({})); + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + mockExit(); + process.argv = ['node', 'bundle-size-summary.js', 'stats.json']; Review Comment: Fixed — saving and restoring `process.argv` in `afterEach` now, alongside `jest.restoreAllMocks()`. ########## superset-frontend/spec/scripts/bundle-size-summary.test.js: ########## @@ -0,0 +1,107 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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. + */ +const fs = require('fs'); +const { + entrypointSizeByExt, + main, +} = require('../../scripts/bundle-size-summary'); + +function mockStats(entrypoints) { + jest + .spyOn(fs, 'readFileSync') + .mockReturnValue(JSON.stringify({ entrypoints })); +} + +function mockExit() { + return jest.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit called'); + }); +} + +afterEach(() => { + jest.restoreAllMocks(); +}); + +test('entrypointSizeByExt sums only assets matching the given extension', () => { + const entrypoint = { + assets: [ + { name: 'spa.entry.js', size: 100 }, + { name: 'spa.entry.js.map', size: 500 }, + { name: 'spa.entry.css', size: 20 }, + ], + }; + expect(entrypointSizeByExt(entrypoint, '.js')).toBe(100); + expect(entrypointSizeByExt(entrypoint, '.css')).toBe(20); +}); + +test('entrypointSizeByExt returns 0 when the entrypoint has no assets', () => { + expect(entrypointSizeByExt({}, '.js')).toBe(0); +}); + +test('main prints byte totals for every tracked entrypoint', () => { + mockStats({ + spa: { assets: [{ name: 'spa.js', size: 100 }] }, + embedded: { assets: [{ name: 'embedded.js', size: 50 }] }, + }); + const logSpy = jest.spyOn(console, 'log').mockImplementation(() => {}); + process.argv = ['node', 'bundle-size-summary.js', 'stats.json']; + + main(); + + const printed = JSON.parse(logSpy.mock.calls[0][0]); + expect(printed).toEqual([ + { name: 'spa entrypoint (JS)', unit: 'bytes', value: 100 }, + { name: 'spa entrypoint (CSS)', unit: 'bytes', value: 0 }, + { name: 'embedded entrypoint (JS)', unit: 'bytes', value: 50 }, + { name: 'embedded entrypoint (CSS)', unit: 'bytes', value: 0 }, + ]); +}); + +test('main exits with an error when a tracked entrypoint is missing from stats.json', () => { + mockStats({ spa: { assets: [] } }); + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + mockExit(); + process.argv = ['node', 'bundle-size-summary.js', 'stats.json']; + + expect(main).toThrow('process.exit called'); + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining('missing the "embedded" entrypoint'), + ); +}); + +test('main exits with an error when stats.json has no `entrypoints` key', () => { + jest.spyOn(fs, 'readFileSync').mockReturnValue(JSON.stringify({})); + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + mockExit(); + process.argv = ['node', 'bundle-size-summary.js', 'stats.json']; + + expect(main).toThrow('process.exit called'); + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining('no `entrypoints` key'), + ); +}); + +test('main prints a usage message and exits when no stats path is given', () => { + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + mockExit(); + process.argv = ['node', 'bundle-size-summary.js']; Review Comment: Fixed — saving and restoring `process.argv` in `afterEach` now, alongside `jest.restoreAllMocks()`. ########## .github/workflows/superset-frontend.yml: ########## @@ -195,3 +195,91 @@ jobs: run: | docker run --rm $TAG bash -c \ "npm run build-storybook && npx playwright install-deps && npx playwright install chromium && npm run test-storybook:ci" + + # Compares a PR's own bundle size against the last nightly-recorded + # baseline (see frontend-bundle-size-nightly.yml, which owns actually + # persisting new baselines). PR-only: a push to master doesn't need this + # check re-run against itself, and re-persisting the baseline on every + # push to master -- which happens many times a day -- would burn a full + # production build for no benefit nightly refresh doesn't already cover. + bundle-size: + needs: frontend-build + if: needs.frontend-build.outputs.should-run == 'true' && github.event_name == 'pull_request' + runs-on: ubuntu-26.04 + timeout-minutes: 15 + permissions: + contents: write + pull-requests: write Review Comment: Fixed — dropped to `contents: read`. Only `pull-requests: write` is needed for `comment-on-alert`; this job never persists (see the comment above it — the nightly job owns the baseline). ########## .github/workflows/superset-frontend.yml: ########## @@ -195,3 +195,91 @@ jobs: run: | docker run --rm $TAG bash -c \ "npm run build-storybook && npx playwright install-deps && npx playwright install chromium && npm run test-storybook:ci" + + # Compares a PR's own bundle size against the last nightly-recorded + # baseline (see frontend-bundle-size-nightly.yml, which owns actually + # persisting new baselines). PR-only: a push to master doesn't need this + # check re-run against itself, and re-persisting the baseline on every + # push to master -- which happens many times a day -- would burn a full + # production build for no benefit nightly refresh doesn't already cover. + bundle-size: + needs: frontend-build + if: needs.frontend-build.outputs.should-run == 'true' && github.event_name == 'pull_request' + runs-on: ubuntu-26.04 + timeout-minutes: 15 + permissions: + contents: write + pull-requests: write + steps: + - name: Checkout Code + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Download Docker Image Artifact + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 + with: + name: docker-image + + - name: Load Docker Image + run: | + zstd -d < docker-image.tar.zst | docker load + + # webpack's persistent filesystem cache (superset-frontend/webpack.config.js) + # turns a warm production build into ~20s instead of several minutes, + # but GH-hosted runners are fresh VMs with nothing carried over between + # jobs -- without restoring it explicitly, every single PR would pay + # the full cold-build cost. Keyed on the same files webpack's own + # `buildDependencies` invalidates on, so a stale cache is never used. + - name: Restore webpack build cache + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: superset-frontend/.temp_cache + key: >- + webpack-prod-cache-${{ hashFiles('superset-frontend/package-lock.json', + 'superset-frontend/babel.config.js', 'superset-frontend/tsconfig.json', + 'superset-frontend/webpack.config.js') }} + + # Only ever pull the last recorded data point off the cache, keyed by + # run ID -- `restore-keys` prefix-matches the most recently created + # entry, which is always the latest nightly run. Absent before the + # first nightly run ever happens; benchmark-action starts a fresh + # history in that case. + - name: Restore bundle size history + uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: bundle-size-history.json + key: bundle-size-history-${{ github.run_id }} + restore-keys: | + bundle-size-history- + + - name: Build production bundle with stats + run: | + mkdir -p ${{ github.workspace }}/superset-frontend/bundle-stats + mkdir -p ${{ github.workspace }}/superset-frontend/.temp_cache + docker run \ + -v ${{ github.workspace }}/superset-frontend/bundle-stats:/app/superset-frontend/bundle-stats \ + -v ${{ github.workspace }}/superset-frontend/.temp_cache:/app/superset-frontend/.temp_cache \ + --rm $TAG \ + bash -c \ + "npm i && BUNDLE_SIZE_STATS=true npm run build -- --json=bundle-stats/stats.json" Review Comment: Leaving as `npm i` — matches the same docker-run pattern the lint and plugins:build steps already use elsewhere in this file. Switching all of them to `npm ci` would be a separate, file-wide change. ########## .github/workflows/frontend-bundle-size-nightly.yml: ########## @@ -0,0 +1,134 @@ +name: Frontend bundle size (nightly baseline + analyzer) + +# Refreshes the bundle-size baseline that superset-frontend.yml's `bundle-size` +# job compares PRs against, and publishes a browsable bundle-analyzer treemap +# report of the same build. Deliberately NOT triggered on every push to +# master: a day-old baseline/report is fine for catching relative +# regressions on PRs and for browsing what's actually in the bundle, and +# building the production bundle on every one of the many pushes master +# gets per day would burn CI time for no benefit a nightly refresh doesn't +# already cover. +on: + schedule: + - cron: "0 6 * * *" + workflow_dispatch: {} + +concurrency: + group: ${{ github.workflow }} + cancel-in-progress: true + +env: + TAG: apache/superset:bundle-size-nightly-${{ github.run_id }} + +permissions: + contents: read + +jobs: + refresh-baseline: + runs-on: ubuntu-26.04 + timeout-minutes: 30 + steps: + - name: "Checkout master" + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + ref: master + + - name: Build Docker Image + run: | + docker buildx build \ + -t $TAG \ + --cache-from=type=registry,ref=apache/superset-cache:3.11-slim-trixie \ + --target superset-node-ci \ + . + + # Same cache the PR-time bundle-size job restores/writes -- webpack's + # persistent filesystem cache turns a warm production build into ~20s + # instead of several minutes. See superset-frontend.yml for the + # matching restore step and why it's keyed this way. + - name: Restore webpack build cache + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: superset-frontend/.temp_cache + key: >- + webpack-prod-cache-${{ hashFiles('superset-frontend/package-lock.json', + 'superset-frontend/babel.config.js', 'superset-frontend/tsconfig.json', + 'superset-frontend/webpack.config.js') }} + + # Only ever pull the last recorded data point off the cache, keyed by + # run ID -- `restore-keys` prefix-matches the most recently created + # entry. Absent on the very first run ever; benchmark-action starts a + # fresh history in that case. + - name: Restore bundle size history + uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: bundle-size-history.json + key: bundle-size-history-${{ github.run_id }} + restore-keys: | + bundle-size-history- + + # BUNDLE_ANALYZER rides along in the same build as BUNDLE_SIZE_STATS -- + # they're independent env-gated additions in webpack.config.js (one + # sets `config.stats`, the other pushes plugins), so one production + # build produces both the numeric stats.json and the analyzer's + # report.html. Only report.html is mounted out, not + # BUNDLE_ANALYZER's sibling `statistics.html` sunburst -- that file is + # documented in webpack.config.js as routinely exceeding 100MB for + # this app (it's .gitignore'd for exactly that reason), too large to + # publish as a static site page. + - name: Build production bundle with stats and analyzer report + run: | + mkdir -p ${{ github.workspace }}/superset-frontend/bundle-stats + mkdir -p ${{ github.workspace }}/superset-frontend/.temp_cache + mkdir -p ${{ github.workspace }}/superset/static/assets + docker run \ + -v ${{ github.workspace }}/superset-frontend/bundle-stats:/app/superset-frontend/bundle-stats \ + -v ${{ github.workspace }}/superset-frontend/.temp_cache:/app/superset-frontend/.temp_cache \ + -v ${{ github.workspace }}/superset/static/assets:/app/superset/static/assets \ + --rm $TAG \ + bash -c \ + "npm i && BUNDLE_SIZE_STATS=true BUNDLE_ANALYZER=true npm run build -- --json=bundle-stats/stats.json" Review Comment: Same reasoning as the PR job — matches the existing `npm i` convention in this repo's docker-run steps rather than diverging just for the new nightly job. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
