feat: add new npm script to compare the build output against master - #1212
Conversation
@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
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- Add detection of old results when executing and a request to remove it
- Path to logs better print complete (support Win/Mac). It is hard to find the location.
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?
|
Please also check AI review Bugs
Robustness
Maintainability
|
|
Since
|
3 items are real to scope for this fixes. 6 of the 12 are false positives — already handled correctly:
Interactive-only design is intended for checking locally. Should I make compatibility to CI? How to test it?It could test at
Because. It's hard to manually swap between the 7 - 9 steps. |
No. For local testing only. At least for now, there's no need for that. |
mu-hun
left a comment
There was a problem hiding this comment.
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'
|
@mu-hun it seems broken usage of
Even
In master it works. Please check. According to docs it must be supported. Also ask llm to perform review against AI Review: PR #1186 — Local downloading of optimization statistics for the buildScope: the PR branch ( The PR itself is solid overall. The findings below are ordered by impact. For each
Origin verified against 1.
|
@Alex-302 Please update the
It works after updated. |
|
I have push new commit to the parent PR, describe this details at ea70a38 |
The 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';
}
} |
|
There might be a bug in the master |
|
Regarding #1186 (comment), We agreed to prevent the simultaneous use of both flags within |
It does not look like a problem. |
|
Okay, I'll allow the combination to Note that |
maximtop
left a comment
There was a problem hiding this comment.
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.
There's nothing local left to reuse. State what actually happens instead: building from whatever's committed.
105th
left a comment
There was a problem hiding this comment.
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
How to runyarn compare-build-outputWhat to testThis 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 Move your local git branch -f master 'feature/#1211'Step 1 – pick a branch to comparePrompt: → pick any branch, e.g. Step 2 – filter selectionPrompt: → answer Y, then enter a small filter ID at the 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:
Steps 4–7→ default answers are fine. Let it run to completion. Step 8 – check the report
Run it again, to test cache reuseSame filter selection as above, Y again at the stats promptExpect:
A third run, with a different filter selection, Y at the stats promptExpect it to skip the reuse question entirely and go straight to: Optional: cleanup checkAt Step 4, answer N to "keep worktrees" (so cleanup runs). After the run finishes:
Edge casesInvalid input at Step 1Enter something out of range or non-numeric (e.g. Expect: Ctrl+C mid-runPress Ctrl+C any time after Step 5, while worktrees/builds are active. Expect:
|
@mu-hun I see only one question - "cached sources", no another questions
|
|
The "cached sources" part points the build command to |
|
I got an error at 7 step. |
The question is not about that phase (old is ok).I don't see steps, mentioned in test description, in the flow. |
|
@zloyden #1212 (comment): You should hard reset the master branch to this branch. Please check #1212 (review). |
|
@mu-hun Why then this info is absent in test guide? |
|
Test cases should be clear and straightforward, with no mysteries. |
I've already mentioned at the first line. Okay, But It seem to easy to miss. updated:
|






Turns the manual copy-paste workflow from
DEVELOPMENT.mdinto a single interactive command: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
fix:, which were not supported in the previous script.