Add baseline-throughput benchmark for dart_skills_lint - #115
Conversation
…6-05-1 Fix complexity exceptions
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a performance benchmark tool for dart_skills_lint, including a README and a Dart script to measure baseline generation throughput. The feedback suggests improving the robustness of argument parsing to handle non-integer inputs and ensuring cross-platform compatibility for absolute path detection in synthetic test data.
|
|
||
| // Error 3 (when errorsPerSkill >= 3): an absolute-path link in the body | ||
| // triggers `check-absolute-paths` (warning, but baseline-recordable). | ||
| final body = errorsPerSkill >= 3 ? '# Test skill\n\n[abs](/etc/hosts)\n' : '# Test skill\n'; |
There was a problem hiding this comment.
Using a hardcoded POSIX-style absolute path like /etc/hosts may not trigger the check-absolute-paths rule when the benchmark is run on Windows, as the underlying path package's isAbsolute check is platform-dependent. To ensure the benchmark produces consistent results across platforms, use p.absolute() to generate a path that is guaranteed to be absolute on the host system.
final body = errorsPerSkill >= 3 ? '# Test skill\n\n[abs](${p.absolute('synthetic-abs-path')})\n' : '# Test skill\n';There was a problem hiding this comment.
Addressed in b366f20: now uses p.absolute('synthetic-abs-path') so the link is absolute on the host OS regardless of platform. Verified locally that with --errors-per-skill 3 the baseline records 3 distinct entries (check-absolute-paths, description-too-long, invalid-skill-name).
Adds tool/dart_skills_lint/bench/baseline_throughput.dart, a standalone script that calls validateSkills(generateBaseline: true) in-process across synthetic skill sizes and prints a wall-clock table. Documents its usage in tool/dart_skills_lint/bench/README.md. The benchmark is local-only (not wired into CI) since wall-clock on hosted runners is too noisy to enforce. Fixes #114
- Fix stale doc comment that pointed at CONTRIBUTING.md; the docs live in
bench/README.md.
- Wrap arg parsing (including int.parse calls and the main loop) in a
single FormatException try/catch, and validate that --runs >= 1 and
--warmup >= 0 so bad input produces a clear message instead of a
crash. _parseSizes now reports unparseable or non-positive entries
with a specific message and rejects an empty list.
- Use p.absolute('synthetic-abs-path') for the errors-per-skill=3
link rather than the hardcoded POSIX path '/etc/hosts', so the
benchmark's absolute-paths rule trigger works the same way on
Windows as on Linux/macOS.
Verified 3 distinct baseline entries are recorded when
--errors-per-skill 3 (check-absolute-paths, description-too-long,
invalid-skill-name).
ca774c4 to
b366f20
Compare
The dart_code_linter `prefer-match-file-name` rule fires on Windows when a file declares a class whose name (after snake_case conversion) doesn't match the file's basename. The previous `_Row` class triggered that rule on the Windows analyze_and_test job, even though macOS and Linux passed clean. Convert `_Row` to `typedef _BenchResult = (...)`, a record. Typedefs aren't class declarations, so the rule has nothing to flag, and the record literal at the return site is just as terse as the old constructor call.
1 participant
Summary
Adds a local-only throughput benchmark for
dart_skills_lintso contributors touching the validation loop or baseline I/O can spot regressions before submitting. Closes #114.tool/dart_skills_lint/bench/baseline_throughput.dart: standalone script that generates synthetic skill directories at multiple sizes, callsvalidateSkills(generateBaseline: true)in-process, and prints a wall-clock table.tool/dart_skills_lint/bench/README.md: how-to-run and option reference, co-located with the script.Why
The O(N²) baseline I/O regression that prompted #113 slipped in because nobody ran the tool against more than ~10 skills. This bench exists so contributors can run a one-liner, see a throughput table, and notice if their change regresses something.
Notes for reviewers
validateSkills(...)(notvalidateSkillsInternal). The public wrapper forwardsgenerateBaseline,quiet, andignoreFileOverride, so using the public API works and avoids triggering theinvalid_use_of_visible_for_testing_memberwarning.bench/, sodart test(which only discoverstest/) ignores it. No exclusion config needed.--errors-per-skillsupports 1-3 by layering distinct rules (invalid-skill-name, thendescription-too-long, thencheck-absolute-paths); higher values clamp with a stderr warning since the de-dup logic in_generateBaselineFilerequires distinct(ruleId, file)tuples.Test plan
dart analyze --fatal-infos— no issuesdart format --output=none --set-exit-if-changed .— cleandart run dart_code_linter:metrics analyze lib— no issuesdart test— 110/110 passeddart run dart_skills_lint:cli— all skills validdart run bench/baseline_throughput.dart --sizes 10,50— completes, sensible tabledart run bench/baseline_throughput.dart --sizes 200— median in tens of ms (85ms on local hardware)🤖 Generated with Claude Code