Skip to content

feat: add new npm script to compare the build output against master - #1212

Merged
mu-hun merged 104 commits into
masterfrom
feature/#1211
Sep 23, 2026
Merged

mu-hun merged 104 commits into
masterfrom
feature/#1211

Conversation

@mu-hun

@mu-hun mu-hun commented Jul 9, 2026 •

Copy link
Copy Markdown
Member

Turns the manual copy-paste workflow from DEVELOPMENT.md into a single interactive command:

yarn compare-build-output

Closes #1211. See the implemented design – #1211 (comment) on that issue for the full step-by-step flow and how it grew past the original proposal (shared optimization-stats cache between the two builds, a run lock, Ctrl+C handling, resumable runs).

Test plan

Please see the test guide comment] below — #1212 (comment) for manual verification steps.

Note

  • The old filters from selected branch doesn't affect to build result. This tool design for comparing build output against difference codebase.
  • About extra commits starting with fix:, which were not supported in the previous script.

@mu-hun
mu-hun requested a review from Alex-302 July 9, 2026 07:16
@mu-hun
mu-hun marked this pull request as draft July 9, 2026 07:17
@mu-hun

mu-hun commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

The new script reported diffs on my end... I will check this issue later and fix it.

@Alex-302 I will keep this PR in draft status until the above problem is fixed. Please take a look for an initial review.

@mu-hun mu-hun left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No diffs were reported after running twice. so, I'm converting this PR as ready for review.

Step 8/9: Report
=== Regression Test Report ===
Branch (reference): master          @ 29ee5260e069fdb10a9873babfb32a213e06f57c
Branch (feature):   feature/#1211 @ 400ad9c11100f9c634aeeb6077bec194eb5d7dc6
Build command:      yarn generate-cache && yarn build:local --no-patches-prepare --strip-generated-meta
Platforms compared: android cli extension ios mac mac_v2 mac_v3 windows 

--- Rule Files ---
Total .txt files compared: 3361
Files with diffs:          0

--- Metadata Files (informational) ---
filters.json/filters.js diffs: 32  (version counter noise, not a regression)

--- Verdict ---
✓ PASS — no rule file diffs

Step 9/9: Cleanup
✓ cleaning up worktrees and build output   
✨  Done in 553.45s.

Comment thread scripts/compare-build-output-against-master.sh Outdated
@mu-hun
mu-hun marked this pull request as ready for review July 10, 2026 03:05
@mu-hun
mu-hun requested a review from zloyden July 10, 2026 03:05

@Alex-302 Alex-302 left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • Add detection of old results when executing and a request to remove it
image
  • Path to logs better print complete (support Win/Mac). It is hard to find the location.
Image

Also I suggested to use temp dir of the repo - in any case it is gitignored.

  • Logs extension must be .log

  • Use cached build (generate-cache + build:local) instead of a plain build?

better Use cached sources instead of a regular build? without (generate-cache + build:local) - it is explained in docs.

  • Step 2/9: Build mode

Use cached build (generate-cache + build:local) instead of a plain build?
[Y/N] (default: Y):

I think the default shoulde No

  • Step 9/9: Cleanup
    → Kept for later use: temp/reg-master-build, temp/reg-changed-build, platforms_master_build, platforms_changed_build

=> Cleanup skipped. + paths.

Btw why platforms_changed_build and platforms_master_build are created in the root of the repo, but not in temp dir or the repo?

@Alex-302

Alex-302 commented Jul 10, 2026 •

Copy link
Copy Markdown
Member

Please also check AI review

Bugs

  • source "$META_FILE" breaks on branch names containing / — CHANGED_BRANCH=feature/ABC-123 is executed as a command by source. Save values with quotes: CHANGED_BRANCH="$CHANGED_BRANCH".
  • printf "\033[%dA" emits raw escape codes in non-TTY — cursor-up sequence [~line 300] is gated by [ -t 1 ], but draw_branch_menu itself is not. In CI / piped output the whole interactive menu would produce garbage. Either abort early when not a TTY or guard the menu entirely.
  • No SIGINT/SIGTERM handler — Ctrl+C during worktree setup, install, or build leaves stale worktrees, platforms_*_build/ dirs, and $META_FILE behind. On re-run the script may silently reuse corrupted state. Add a trap that cleans up worktrees, output dirs, $META_FILE, and $LOCK_DIR.
  • No local check for $BASED_BRANCH — git rev-parse "$BASED_BRANCH" fails if the branch (master) isn't fetched locally. Should check or fall back to origin/master.

Robustness

  • Reused worktree skips yarn install even if deps changed — Step 5 only installs when node_modules/ is missing. If a worktree is reused (Step 4 user says "yes") but package.json / yarn.lock differ from the detached commit, the build will use stale dependencies. Compare lockfile hashes before skipping.
  • report_failure + exit 1 pattern repeated 5+ times — every step duplicates report_failure "... FAILED" "$log"; exit 1. Extract into a helper like run_or_fail(label, log, ...cmd).
  • No set -euo pipefail — some command failures may go undetected. Script currently relies on manual $? checks, which are thorough but not exhaustive.

Maintainability

  • Build flags hardcoded in two places — --no-patches-prepare --strip-generated-meta appears in build_branch() and in generate_report(). Extract to a single variable.
  • TOTAL_STEPS=9 is never used outside step_header — could just hardcode 9 in the header calls, or use $TOTAL_STEPS consistently in all references including step_header's own format string.
  • draw_branch_menu is called unconditionally — even when $CURRENT_BRANCH is empty (detached HEAD) the full menu renders. Works but the "no default" message on line ~261 only warns, doesn't fall back to a sensible default like the first branch.
  • No shellcheck validation in CI — the script has # shellcheck disable=SC1090 for the source "$META_FILE" line but shellcheck is not part of yarn lint. Consider adding it or at least running shellcheck manually.
  • Interactive-only design — no --branch, --cached, --no-cleanup CLI flags. Makes it unusable in CI. Low priority if CI has its own pipeline.

@mu-hun

mu-hun commented Jul 14, 2026 •

Copy link
Copy Markdown
Member Author

Since --generate-cache and --download-cache are doesn't require for --use-cache from our latest discussions, I will update add new steps:

  • ask to run --generate-cache
  • ask to run --download-stats

Comment thread scripts/compare-build-output-against-master.sh Outdated
@mu-hun

mu-hun commented Jul 23, 2026 •

Copy link
Copy Markdown
Member Author

reply to #1212 (comment)

3 items are real to scope for this fixes. ec3fe51 (this PR), 728f3c6 (this PR), 4273084 (this PR)

6 of the 12 are false positives — already handled correctly:

  • source "$META_FILE" with an unquoted branch name containing / and # (e.g. feature/#1211) sources fine — verified with a standalone repro; # only starts a comment at the start of a word, not mid-token after =.
  • Non-TTY escape-code leakage — every printf with a raw escape sequence in draw_branch_menu/the arrow-key loop is already gated by [ -t 1 ] or uses the C_* vars (empty when not a TTY); verified with piped/</dev/null repro producing clean output.
  • No SIGINT/SIGTERM handler — the existing trap 'rm -rf "$LOCK_DIR"' EXIT already fires on SIGINT (verified: bash runs EXIT traps on signal termination). Worktrees/platform dirs intentionally persist across an interrupted run (same as the "cleanup skipped" resumable design) and are never silently reused as corrupt state, since $META_FILE — the sole gate for Step 0's reuse offer — is only written after both builds fully succeed.
  • TOTAL_STEPS claim is self-contradictory (already used in step_header's own format string).
  • Detached-HEAD branch-menu fallback already defaults SELECTED_INDEX=0 (first branch) when no current-branch match is found.
  • set -euo pipefail too risky against the script's background-job/expected-failure patterns for the benefit, given manual checks already cover every critical path — if-checks, including patterns that would break under pipefail/errexit.

Interactive-only design is intended for checking locally. Should I make compatibility to CI?

How to test it?

It could test at e968794 (this PR) but after merge commit testing is backlog.

  1. git reset --hard e96879475a2456818be049613f610c93cb3256ad
  2. yarn compare-build-output Please select the feature/#1211-test branch at first step.

[skip ci] I'm planning to run after next released FiltersCompiler version.

checkout unreleased FiltersCompiler's build and move it to each node_module (git worktrees)

Because. It's hard to manually swap between the 7 - 9 steps.

@mu-hun
mu-hun requested a review from Alex-302 July 23, 2026 04:31
@Alex-302

Copy link
Copy Markdown
Member

Interactive-only design is intended for checking locally. Should I make compatibility to CI?

No. For local testing only. At least for now, there's no need for that.

@mu-hun mu-hun left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The regression test script is now runnable.

Step 9/11: Build both branches
✓ [master] build   ✓ [feature/#1211] build   
✗ [master] build FAILED — see /Users/muhun/github.com/AdguardTeam/FiltersRegistry/temp/logs/master-build.log
--- /Users/muhun/github.com/AdguardTeam/FiltersRegistry/temp/logs/master-build.log ---
(node:20997) [DEP0169] DeprecationWarning: `url.parse()` behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for `url.parse()` vulnerabilities.
(Use `node --trace-deprecation ...` to show where the warning was created)
error Command "download-stats" not found.

Please clear the yarn cache for the filters-compiler version to refresh the unreleased development version — yarn cache clean @adguard/filters-compiler. Without doing this, you will encounter the following error:

import { compile, localOptimizationStatistics, OptimizationStatsError } from '@adguard/filters-compiler';
                                               ^^^^^^^^^^^^^^^^^^^^^^
SyntaxError: The requested module '@adguard/filters-compiler' does not provide an export named 'OptimizationStatsError'

Comment thread scripts/compare-build-output-against-master.sh Outdated
@Alex-302

Alex-302 commented Aug 11, 2026 •

Copy link
Copy Markdown
Member

@mu-hun it seems broken usage of include + skip yarn build --include=1,2,3 --skip=2

image

Even include=1,2,3 does not work

image

In master it works. Please check. According to docs it must be supported. Also ask llm to perform review against master (as I did. You can do this before publishing changes and, accordingly, fix any confirmed issues).


AI Review: PR #1186 — Local downloading of optimization statistics for the build

Scope: the PR branch (local_optimization_config, merged into feature/#1211) was
reviewed against @adguard/filters-compiler@3.2.12. Verified locally:
yarn test (71/71 pass) and yarn lint (code + types + markdown) are green.

The PR itself is solid overall. The findings below are ordered by impact. For each
one: what the problem is, what it can lead to, whether it is a regression, and
whether it exists in master (origin: introduced by PR #1186 vs pre-existing).

# Finding Severity Regression? Origin
1 --include + --skip become mutually exclusive in every mode, docs contradict code High Yes Introduced by PR #1186
2 filterId lost when wrapping OptimizationStatsError Medium No Introduced by PR #1186
3 Partial snapshot pitfall: scoped download leaves stale stats behind Medium No Introduced by PR #1186
4 "Offline" build is not actually offline (percent.json is always remote) Medium No Mixed: behavior pre-existing, claim new
5 --report silently ignored under --download-stats Low No Introduced by PR #1186
6 Test mock missing OptimizationStatsError; error path untested Low No Introduced by PR #1186
7 Dangerous git reset --hard in the documented cleanup steps Low No Introduced by PR #1186
8 Bash script placed inside the Vitest __tests__/ directory Low No Introduced by PR #1186

Origin verified against master (git): none of these findings exist there — with one
nuance, item 4. Master has no local optimization stats at all (everything —
percent.json and stats.json — is fetched from the remote server by the compiler),
so items 2, 3, 5, 6, 7 and 8 concern code, tests, and docs that are entirely new in
this PR. For item 4, the underlying behavior (percent.json always fetched remotely)
is inherited from the compiler and was already true in master; what the PR adds is
the "offline" claim that contradicts it.


1. --include + --skip become mutually exclusive in every mode (docs contradict code)

Essence. The new check in validateFlags() (scripts/build/build-config.ts) rejects
--include together with --skip for all commands — a plain yarn build,
--generate-cache, --use-cache, and --download-stats alike. Before this PR, passing
both was legal: the compiler applied them as intersection minus exclusion (a filter is
built only if it is in the include list AND not in the skip list). Meanwhile
DEVELOPMENT.md still documents that combination as valid:

yarn build --include=1,2,3 --skip=2 # intersection minus exclusion. Excessive, but it works

and the "Invalid combinations" section only mentions the restriction under
--download-stats. So the code and the docs contradict each other.

What it can lead to. A previously working, documented command now exits with an error.
Anyone with such a command in a script, CI step, or muscle memory gets a hard failure.
The docs mislead readers about what is valid.

Regression? Yes. This is a breaking CLI behavior change against previously
documented behavior, and the compiler still supports the combination. Minimal fix:
restrict the check to --download-stats only (the compiler throws there anyway, the
registry-side check just produces a nicer message), or keep it global but update both
doc sections (valid + invalid lists) in DEVELOPMENT.md.


2. UX error: filterId lost when wrapping OptimizationStatsError

Essence. When a local stats snapshot is incomplete, build.js catches
OptimizationStatsError and rethrows a generic message:

if (useCache && error instanceof OptimizationStatsError) {
    throw new Error('Run --download-stats to download the latest statistics.', { cause: error });
}

The compiler's error type was extended (in this same feature) to carry structured fields
— filterId and sourcePath — precisely so the registry could tell the user which
filter is missing, instead of matching on error.message. But build.js discards both
fields and prints only the generic hint.

What it can lead to. With a partial snapshot (e.g. after a scoped
yarn download-stats --include=...), the build fails without saying which filter is
missing. The user must manually diff the downloaded files against the filter list to
figure it out — exactly the friction the structured error was meant to remove.

Regression? No. New code, not a regression — it is a lost opportunity: the
compiler-side groundwork exists but is unused. Fix is small: include error.filterId
(and error.sourcePath) in the rethrown message.


3. Partial snapshot pitfall: scoped download leaves stale stats behind

Essence. yarn download-stats --include=1,2,3 overwrites only the selected
stats.json files. Any older stats.json files for other filters stay on disk. old stats for some filters,
new stats for others. If a filter has no stats file at all, the build fails with the "Run --download-stats" error.

What it can lead to. Silent, mixed-version snapshots: the build is reproducible
against a snapshot that never existed as a whole. Optimized output for some filters is
computed from stale statistics, so local results can diverge from production for no
obvious reason. Debugging is confusing: "I just downloaded stats — why does filter 4
fail?"

Regression? No. New feature; the behavior is partially documented ("Existing
stats.json files are overwritten"), but the pitfall is not. Fix options: clear the
stats directory before downloads, or explicitly warn in the docs that a scoped
download leaves a partial cache that affects subsequent build:local runs.


4. "Offline" build is not actually offline (percent.json is always remote)

Essence. The PR description promises "building filters offline against a pinned stats
snapshot". In reality only per-filter stats.json files are read from disk.
percent.json — the file that defines which filters are optimizable and their targets —
is fetched from chrome.adtidy.org on every compile, even when
localOptimizationStatistics.use() was called. The registry's own test acknowledges
this: "compile() always fetches percent.json remotely, even under use()."

What it can lead to. On a machine without network access, yarn build:local fails at
the percent.json fetch — the advertised offline workflow does not work. Also, because
percent.json changes remotely, two builds of the same pinned stats snapshot can differ
(the set of optimizable filters is not pinned).

Regression? No. This behavior predates the PR: in master the compiler already
fetched percent.json (and everything else) from the remote server — the registry had
no local stats at all. So the root behavior is pre-existing; what the PR introduces is
only the "offline" claim that contradicts it. Note that not writing percent.json
locally is a deliberate design choice ("it can't be edited locally") — but then the
docs should stop implying full offline builds.


Minor issues

5. --report silently ignored under --download-stats

Essence. --download-stats --report=foo.txt passes validation, but no report file is
ever written (reports are only created by the compile step).

What it can lead to. Do rejects when --download-stats combined with --report

Regression? No. New flag combination; should be rejected and documented.

6. OptimizationStatsError thrown test case missing.

Essence. The catch branch (error instanceof OptimizationStatsError) is never
exercised by any test.

Regression? No. New tests; they pass today, but they are fragile and incomplete.
Fix: add a test for the OptimizationStatsError.

7. Dangerous git reset --hard in the documented cleanup steps This is ok for us, leave as is

Essence. DEVELOPMENT.md ends the "compare against master" workflow with:

git worktree remove /tmp/reg-master-build -f & git worktree remove /tmp/reg-changed-build -f
git reset --hard && rm -rf platforms_master_build platforms_changed_build

All branch-specific changes live in the /tmp worktrees; the main checkout is never
modified by the workflow. So git reset --hard in the main repo discards nothing related
to it — but it destroys any uncommitted work the developer has in the main working tree.

What it can lead to. Data loss for anyone who follows the doc while having
uncommitted changes in the main checkout.

Regression? No. New documentation, but the instruction is unsafe. rm -rf of the
two output directories is sufficient; git reset --hard should be removed.

8. Bash script placed inside the Vitest __tests__/ directory

Essence. scripts/build/__tests__/regression-test-against-master.sh is a Bash script
sitting inside a Vitest test directory. It is not a Vitest test, is not run by yarn test, and is easy to mistake for one.

What it can lead to. Confusion about what the directory contains; a future cleanup of
"test files" could delete it. (Already addressed in the follow-up PR #1212, which moves
it to scripts/compare-build-output-against-master.sh.)

Regression? No. Organizational nit.


Bottom line

None of the findings block merging: tests and lint pass, and the core feature works as
intended. Worth fixing before/right after merge:

  1. #1 (docs/code contradiction) — must be resolved before merge, it is the only
    true regression
    this PR introduces: a previously working, documented command
    (yarn build --include=… --skip=…) now errors out.
  2. #2, #3, #4 — small code/doc changes that remove real user-facing footguns.
    Of these, only #4 has a pre-existing root cause (the compiler always fetched
    percent.json remotely); #2 and #3 are entirely new code from this PR.
  3. #5–#8 — nice-to-haves, mostly documentation and test hygiene; none of them exist
    in master (the code, tests, and docs they concern are all new in this PR).

Origin summary: compared against master, everything in this review except the
percent.json behavior in #4 is new in PR #1186. Master has no local optimization
stats at all — it always fetched percent.json and stats.json from the remote
server — so items 2, 3, 5, 6, 7 and 8 cannot exist in master, and item 4's underlying
behavior predates the PR.

@mu-hun

mu-hun commented Aug 11, 2026 •

Copy link
Copy Markdown
Member Author

it seems broken usage of include + skip yarn build --include=1,2,3 --skip=2

@Alex-302 Please update the node_modules. After updating, It will throw the Error: --include and --skip are mutually exclusive as described in first review section — "--include + --skip become mutually exclusive in every mode (docs contradict code)"

FiltersRegistry on feature/#1211 [$!] is 📦 v1.1.0 via v26.0.0 took 48s 
❯ yarn build --include=1,2,3 --skip=2
$ tsx scripts/build/build.js --include=1,2,3 --skip=2

Error: --include and --skip are mutually exclusive.
See Command Compatibility in DEVELOPMENT.md for valid combinations.

Even include=1,2,3 does not work

It works after updated.

FiltersRegistry on feature/#1211 [$!] is 📦 v1.1.0 via v26.0.0 took 2s 
❯ yarn build --include=1,2,3         
$ tsx scripts/build/build.js --include=1,2,3
(node:74365) [DEP0205] DeprecationWarning: `module.register()` is deprecated. Use `module.registerHooks()` instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
2026-08-11T18:34:59:836: Using custom platforms configuration
2026-08-11T18:34:59:840: Redefining platform WINDOWS

@Alex-302

Copy link
Copy Markdown
Member

It works after updated.

Weird, sure. But modules were frest

image

become mutually exclusive

Must work. In build plan skip can be used to temporary build some filters. With a separate setting, there is less risk of forgetting to re-enable the excluded filter. In general no resons to break this feature, at least for general usage.

image

@mu-hun

mu-hun commented Aug 11, 2026

Copy link
Copy Markdown
Member Author

Error: --include and --skip are mutually exclusive. This new validation was implemented following our discussion, where it was agreed to prevent the simultaneous use of both flags within the --download-stats command.

#1186 (comment): The generate-cache and use-cache flags are currently not being rejected when --include and --skip are used concurrently. Should we modify the logic to reject the input when both flags are provided simultaneously, as proposed in this change?

I have push new commit to the parent PR, describe this details at ea70a38

@mu-hun

mu-hun commented Aug 11, 2026

Copy link
Copy Markdown
Member Author
  1. UX error: filterId is omitted when wrapping an OptimizationStatsError.

The filterId is logged as Unable to retrieve optimization stats for ${filterId}, at ${sourcePath}... within the FiltersCompiler component.

Based on the current source code implementation:

export class OptimizationStatsError extends Error {
    code = 'OPTIMIZATION_STATS_UNAVAILABLE' as const;

    constructor(
        public filterId: number,
        public sourcePath: string,
        options?: ErrorOptions,
    ) {
        super(
            `Unable to retrieve optimization stats for ${filterId}, at ${sourcePath}. `
            + 'Please ensure the stats file exists and is accessible.',
            options,
        );
        this.name = 'OptimizationStatsError';
    }
}

@Alex-302

Alex-302 commented Aug 13, 2026 •

Copy link
Copy Markdown
Member

@mu-hun

I have push new commit to the parent PR, describe this details at ea70a38

You implemented regression. The fact that you've documented a change in behavior doesn't solve the problem. include and skip must must be compatible at least with prod commands.

In master they can be combined

image

@Alex-302

Copy link
Copy Markdown
Member

There might be a bug in the master
yarn build --generate-cache --include=1,2,3 --skip=2
downloaded only 1, but not 3

@mu-hun

mu-hun commented Aug 13, 2026 •

Copy link
Copy Markdown
Member Author

Regarding #1186 (comment), We agreed to prevent the simultaneous use of both flags within --generate-cache and --use-cache. Should I revert this decision?

@Alex-302

Copy link
Copy Markdown
Member

@mu-hun

We agreed to prevent the simultaneous use of both flags within --generate-cache and --use-cache. Should I revert this discussion?

It does not look like a problem. --generate-cache + --use-cache is like a regular build without parameters.

@mu-hun

mu-hun commented Aug 13, 2026 •

Copy link
Copy Markdown
Member Author

Okay, I'll allow the combination to --generate-cache, --download-stats, and --use-cache flags.

Note that --download-stats requires a change on the FilterCompiler side. Please wait.

@Alex-302

Alex-302 commented Aug 13, 2026 •

Copy link
Copy Markdown
Member

There might be a bug in the master
yarn build --generate-cache --include=1,2,3 --skip=2
downloaded only 1, but not 3

@mu-hun Please check #1223
AI generated, but looks ok.

It seems actual only for Windows with Powershell terminal. In bash it works. Assigned low priority.

@mu-hun mu-hun left a comment •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@Alex-302 Hi, I've improved some step descriptions based on your suggestion.

Note: Please leave any change requests and suggestions about comparing scripts in this PR, not in the child PRs.

Comment thread scripts/compare-build-output-against-master.sh Outdated
Comment thread scripts/compare-build-output-against-master.sh Outdated
@mu-hun
mu-hun requested review from Alex-302 and removed request for Alex-302 August 26, 2026 03:22

@maximtop maximtop left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Went through the latest head end to end — the shared-stats lifecycle, the cleanup paths and the report now line up. A few leftovers below, none of them blocking.

Comment thread DEVELOPMENT.md Outdated
Comment thread scripts/compare-build-output-against-master.sh Outdated
Comment thread scripts/compare-build-output-against-master.sh Outdated
Comment thread scripts/compare-build-output-against-master.sh
Comment thread scripts/build/build.js Outdated
Comment thread .eslintignore Outdated
Comment thread scripts/compare-build-output-against-master.sh
Comment thread scripts/compare-build-output-against-master.sh Outdated

@105th 105th left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

went through the latest commits — stats are back to cached-only, and the prompts, report and meta line up. rebase before merge: package.json / .markdownlintignore conflict with master, and master moved filters-compiler to 3.3.2-beta.0

@mu-hun

mu-hun commented Sep 22, 2026 •

Copy link
Copy Markdown
Member Author

How to run

yarn compare-build-output

What to test

This PR's only new behavior is in Step 3: optimization stats are now downloaded once and shared between both worktrees, and a second run can reuse them instead of re-downloading. Everything else in the script is unchanged and already covered by earlier review rounds.

Until this PR merges, comparing against the real master branch will fail or produce wrong results: the new optimization-stats scope marker this script relies on is written by build.js on this branch, not yet on master (see this review thread).

Move your local master to this branch first — you're already on this branch to run the script, so this doesn't touch your checkout:

git branch -f master 'feature/#1211'

Step 1 – pick a branch to compare

Prompt: Select a branch [1-N] (default: ...) — N is however many local branches you have, not a fixed number.

→ pick any branch, e.g. feature/#1211 current branch

Step 2 – filter selection

Prompt: Use filter selection?

→ answer Y, then enter a small filter ID at the Filter IDs to build prompt (e.g. 2) for a fast run.

Step 3 – build mode (the step under test)

This step asks up to three questions, but the 2nd and 3rd only appear if you answer Y to the first one:

  1. Use yarn build:local instead of regular build? → Y (required — answering N skips the rest of this step, including the stats-cache question below)
  2. Generate filter.txt cache (yarn generate-cache)? → Y or N, doesn't matter for this test
  3. Use local optimization stats cache (shared across both builds)? → Y
    • First time you run this on a clean temp/, expect: No shared stats found; will download before building
    • If shared stats already exist (see "Run it again" below), expect one of the "Found existing shared stats..." messages instead

Steps 4–7

→ default answers are fine. Let it run to completion.

Step 8 – check the report

  • Build command: includes a single yarn download-stats ... prefix (not one per branch)
  • Optimization stats: reads downloaded to temp/reg-stats, shared with both builds

Run it again, to test cache reuse

Same filter selection as above, Y again at the stats prompt

Expect:

  • Found existing shared stats at temp/reg-stats (same filter selection).
  • A new prompt: Reuse them (downloaded from yarn download-stats) before building? → answer Y
  • Step 7 should not show a "downloading shared optimization stats" spinner this time — only "copying shared stats into both worktrees"
  • Step 8 report: Optimization stats: reused existing shared cache at temp/reg-stats

A third run, with a different filter selection, Y at the stats prompt

Expect it to skip the reuse question entirely and go straight to: Existing shared stats at temp/reg-stats were downloaded under a different filter selection; will refresh before building

Optional: cleanup check

At Step 4, answer N to "keep worktrees" (so cleanup runs). After the run finishes:

  • the worktrees and platforms_*_build dirs under temp/ are gone
  • temp/reg-stats is not deleted — it's a persistent cache meant to survive across runs

Edge cases

Invalid input at Step 1

Enter something out of range or non-numeric (e.g. 99 or abc).

Expect: ✗ Enter a number between 1 and N. (repeats until you enter a valid number, doesn't crash)

Ctrl+C mid-run

Press Ctrl+C any time after Step 5, while worktrees/builds are active.

Expect:

  • Interrupted — stopping builds.
  • no orphaned yarn/node processes left behind (ps aux | grep yarn shows nothing after)
  • if cleanup was enabled (Step 4 → "keep worktrees: no"), cleanup still runs on the way out:
    ⠧ cleaning up worktrees and build output
  • if cleanup was disabled, worktrees/output are left in place instead, same as a normal completed run with cleanup off

@Alex-302

Alex-302 commented Sep 22, 2026 •

Copy link
Copy Markdown
Member

Step 3: answer Y to "cached sources", Y to "generate filter.txt cache" (or N, doesn't matter), then Y to "Use local optimization stats cache?".

  1. First time, expect: No shared stats found; will download before building
  2. Second+ time, expect 5-1

@mu-hun I see only one question - "cached sources", no another questions

image

@mu-hun

mu-hun commented Sep 22, 2026

Copy link
Copy Markdown
Member Author

The "cached sources" part points the build command to yarn build:local. I'm going to rename the title to "Use yarn build:local instead of regular build?"

@zloyden

zloyden commented Sep 22, 2026

Copy link
Copy Markdown
Member

I got an error at 7 step.

Step 7/9: Build both branches
✓ downloading shared optimization stats
✓ copying refreshed stats
✗ download-stats succeeded but wrote no .scope marker at

@Alex-302

Copy link
Copy Markdown
Member

The "cached sources" part points the build command to yarn build:local. I'm going to rename the title to "Use yarn build:local instead of regular build?"

The question is not about that phase (old is ok).I don't see steps, mentioned in test description, in the flow.

@mu-hun

mu-hun commented Sep 22, 2026

Copy link
Copy Markdown
Member Author

@zloyden #1212 (comment): You should hard reset the master branch to this branch. Please check #1212 (review).

@Alex-302

Copy link
Copy Markdown
Member

@mu-hun Why then this info is absent in test guide?

@Alex-302

Copy link
Copy Markdown
Member

Test cases should be clear and straightforward, with no mysteries.

@mu-hun

mu-hun commented Sep 22, 2026 •

Copy link
Copy Markdown
Member Author

#1212 (review): Before testing, You should hard reset the master branch to this branch.

I've already mentioned at the first line. Okay, But It seem to easy to miss.

updated:

Until this PR merges, comparing against the real master branch will fail or produce wrong results: the new optimization-stats scope marker this script relies on is written by build.js on this branch, not yet on master (see this review thread).

Move your local master to this branch first — you're already on this branch to run the script, so this doesn't touch your checkout:

git branch -f master 'feature/#1211'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create a regression test script to compare build results against master

7 participants