Skip to content

ci: avoid counters already excluded from coverage reports - #470

Merged
XuPeng-SH merged 1 commit into
matrixorigin:mainfrom
XuPeng-SH:fix/coverage-counter-memory
Oct 26, 2026
Merged

XuPeng-SH merged 1 commit into
matrixorigin:mainfrom
XuPeng-SH:fix/coverage-counter-memory

Conversation

@XuPeng-SH

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

Related to matrixorigin/matrixone#29562 and matrixorigin/matrixone#31146.

What this PR does / why we need it:

The coverage report already discards generated protobuf/goyacc records after collecting them. Apply the same policy before instrumentation, keeping the complete test-package selection. In the measured main inventory this runs all 505 test packages and instruments 484. Tests inside excluded packages still contribute hits to retained dependencies.

Reuse the existing compactor as the sole exclusion-policy owner. Compile it once during bounded preparation and reuse that binary for final compaction. Empty/invalid scope, build failure and profile errors remain fatal. Keep package parallelism 8, cluster admission pool 2, the 20-minute package timeout and existing producer supervision.

Validation:

  • Helper tests pass under race, including a real four-package Go fixture checking excluded-package tests, cross-package hits, untested zero-count blocks and byte-identical published coverage. Go vet, actionlint and Bash syntax checks pass.
  • Local complete embed/arrowload/plan/moerr comparisons preserve all 20,768 passing and 6 existing skipped test terminals, plus the 398,105 retained coverage-block keys. Raw profile output falls from 122,379,126 to 95,869,828 bytes (21.7%).
  • Two warmed comparisons include tests and compaction: 442.7 -> 410.9 seconds with original test timing; 352.9 -> 347.9 seconds with the same shorter embedded-test cadence on both sides. The latter CPU time is 202.5 -> 172.9 seconds.
  • Peak-memory limitation: working-set peaks did not improve: 3.79 -> 5.16 GiB in the first pair and 3.67 -> 3.89 GiB in the second. This change removes unnecessary instrumentation/CPU/profile I/O; it is not claimed as a peak-memory fix. Whole-job timing and memory for all 505 packages remain for CI to establish.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deep review of 1a19690 against d725189: no blocking findings.

The existing report exclusion predicate remains the single policy owner. The new mode narrows only -coverpkg; the unchanged test_scope still reaches go test. Verified against matrixone#31237's actual package inventory: 505 test packages, 484 instrumented packages, excluding only 20 pkg/pb packages plus goyacc. Excluded-package tests still execute. Helper checkout is pinned to job.workflow_sha, and the same compiled helper is reused for selection and compaction. Build/selection errors stop the phase; test supervision and compaction failure handling remain intact.

Independent validation:

  • All seven helper tests pass locally under -race -count=1 (4.818s); go vet, formatting and diff checks pass.
  • The real four-package Go fixture proves excluded-package tests and their hits in retained dependencies still execute, untested zero-hit blocks remain in the denominator, and compacted old/new profiles are byte-identical. Empty/invalid scope and read/write failures are covered.
  • Exact-head CI job 121429185175 passed; its log confirms actionlint plus helper tests/vet executed.

Complexity is proportionate: helper net +36, workflow net +8, tests +94, no new documentation/evidence files. The added test uses a small real Go module rather than duplicating coverage behavior in a mock.

Performance boundary: this removes redundant counters/profile records. The PR's measurements do not establish a reduction in peak memory; the recorded memory peaks increased in those samples. No claim of full-job memory improvement or complete 505-package combined performance validation is made. MatrixOne's currently running workflow uses CI@main, so it is not an end-to-end run of this unmerged CI revision. The changes have no hard merge-order dependency.

No existing review threads were present at recheck. Submitting COMMENT because the authenticated account is also the PR author.

@XuPeng-SH
XuPeng-SH enabled auto-merge (squash) October 26, 2026 07:25
@XuPeng-SH
XuPeng-SH disabled auto-merge October 26, 2026 07:26
@XuPeng-SH
XuPeng-SH merged commit 61087f3 into matrixorigin:main Oct 26, 2026
1 check passed
@XuPeng-SH
XuPeng-SH deleted the fix/coverage-counter-memory branch October 26, 2026 07:26
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.

1 participant