Skip to content

*: switch bazel coverage test targets to bazel test#67982

Open
hawkingrei wants to merge 1 commit intopingcap:masterfrom
hawkingrei:bazel-coverage-test-to-test
Open

*: switch bazel coverage test targets to bazel test#67982
hawkingrei wants to merge 1 commit intopingcap:masterfrom
hawkingrei:bazel-coverage-test-to-test

Conversation

@hawkingrei
Copy link
Copy Markdown
Member

@hawkingrei hawkingrei commented Apr 23, 2026

What problem does this PR solve?

Issue Number: ref #67956

Problem Summary:

bazel_coverage_test and bazel_coverage_test_ddlargsv1 still invoke bazel coverage, which keeps the target behavior tied to coverage report generation even when the caller only needs a normal Bazel test run.

What changed and how does it work?

  • Switched both Makefile targets from bazel coverage to bazel test
  • Removed --combined_report=lcov from both targets
  • Kept the remaining Bazel arguments unchanged

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
make -n bazel_coverage_test
make -n bazel_coverage_test_ddlargsv1
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Summary by CodeRabbit

Release Notes

  • Chores
    • Updated test coverage execution process in build system to streamline the testing workflow.

@ti-chi-bot ti-chi-bot Bot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 23, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 23, 2026

📝 Walkthrough

Walkthrough

The bazel_coverage_test and bazel_coverage_test_ddlargsv1 Make targets are modified to execute bazel test instead of bazel coverage, with the --combined_report=lcov option removed while preserving all other Bazel configuration variables and flags.

Changes

Cohort / File(s) Summary
Bazel Coverage Test Targets
Makefile
Modified two Bazel-based Make targets to replace bazel coverage with bazel test and removed --combined_report=lcov option; preserved existing config variables, test patterns, and job flags.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • pingcap/tidb#67634 — Adjusts Jenkins scripts' handling of Bazel coverage artifacts in relation to how make bazel_coverage_test* is invoked.
  • pingcap/tidb#67736 — Modifies the same Makefile Bazel targets with concurrent job adjustments alongside the command transition from bazel coverage to bazel test.

Suggested labels

size/XS, approved, lgtm

Suggested reviewers

  • winoros
  • lance6716

Poem

🐰 We hop from coverage to test commands anew,
Bazel bounds faster with test in lieu,
No combined reports to weigh us down,
Faster runs wear the testing crown! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: switching bazel coverage test targets from 'bazel coverage' to 'bazel test'.
Description check ✅ Passed The description includes most required sections with adequate detail: issue reference, problem summary, explanation of changes, and test verification, though the problem statement could be more explicit about the rationale.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Makefile`:
- Around line 718-725: CI currently expects the old coverage artifact produced
by the bazel_coverage_test* targets, but the changed bazel test invocation in
target bazel_coverage_test_ddlargsv1 no longer emits that report; update either
the two CI scripts (jenkins_unit_test.sh and jenkins_unit_test_ddlargsv1.sh) to
detect the new behavior and consume whatever coverage output Bazel now produces
(or gracefully handle missing coverage), or restore a separate
coverage-producing Bazel target (e.g., reintroduce a bazel_coverage_test target
variant that generates the legacy coverage report) and wire that target back
into the CI callers so the expected artifact is present.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: c88ee881-a9ee-43cd-95f5-29c4ff732ca3

📥 Commits

Reviewing files that changed from the base of the PR and between 5260244 and 8b0e99c.

📒 Files selected for processing (1)
  • Makefile

Comment thread Makefile
Comment on lines +718 to +725
bazel $(BAZEL_GLOBAL_CONFIG) --nohome_rc test $(BAZEL_CMD_CONFIG) $(BAZEL_INSTRUMENTATION_FILTER) --jobs=HOST_CPUS*0.9 --build_tests_only --test_keep_going=false \
--define gotags=$(UNIT_TEST_TAGS) \
-- //... -//cmd/... -//tests/graceshutdown/... \
-//tests/globalkilltest/... -//tests/readonlytest/... -//tests/realtikvtest/...

.PHONY: bazel_coverage_test_ddlargsv1
bazel_coverage_test_ddlargsv1: bazel-failpoint-enable bazel_ci_simple_prepare
bazel $(BAZEL_GLOBAL_CONFIG) --nohome_rc coverage $(BAZEL_CMD_CONFIG) $(BAZEL_INSTRUMENTATION_FILTER) --jobs=HOST_CPUS*0.9 --build_tests_only --test_keep_going=false \
--combined_report=lcov \
bazel $(BAZEL_GLOBAL_CONFIG) --nohome_rc test $(BAZEL_CMD_CONFIG) $(BAZEL_INSTRUMENTATION_FILTER) --jobs=HOST_CPUS*0.9 --build_tests_only --test_keep_going=false \
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Update the Jenkins coverage collectors with this behavior change.

build/jenkins_unit_test.sh and build/jenkins_unit_test_ddlargsv1.sh still call these bazel_coverage_test* targets and look for ./bazel-out/_coverage/_coverage_report.dat. With bazel test, that report will no longer be produced, so CI will fall back to an empty coverage.dat and may silently drop coverage data. Please update those scripts/artifact expectations in the same PR if losing coverage is intentional, or keep a separate coverage-producing target for coverage jobs.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Makefile` around lines 718 - 725, CI currently expects the old coverage
artifact produced by the bazel_coverage_test* targets, but the changed bazel
test invocation in target bazel_coverage_test_ddlargsv1 no longer emits that
report; update either the two CI scripts (jenkins_unit_test.sh and
jenkins_unit_test_ddlargsv1.sh) to detect the new behavior and consume whatever
coverage output Bazel now produces (or gracefully handle missing coverage), or
restore a separate coverage-producing Bazel target (e.g., reintroduce a
bazel_coverage_test target variant that generates the legacy coverage report)
and wire that target back into the CI callers so the expected artifact is
present.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.1182%. Comparing base (874ff37) to head (8b0e99c).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #67982        +/-   ##
================================================
- Coverage   77.7953%   77.1182%   -0.6772%     
================================================
  Files          1984       1966        -18     
  Lines        549728     551723      +1995     
================================================
- Hits         427663     425479      -2184     
- Misses       121145     126105      +4960     
+ Partials        920        139       -781     
Flag Coverage Δ
integration 40.8663% <ø> (+1.0691%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 60.4888% <ø> (ø)
parser ∅ <ø> (∅)
br 49.9988% <ø> (-13.0317%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hawkingrei
Copy link
Copy Markdown
Member Author

Merge before leaving work

/hold

@ti-chi-bot ti-chi-bot Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2026
Comment thread Makefile
bazel_coverage_test: bazel-failpoint-enable bazel_ci_simple_prepare
bazel $(BAZEL_GLOBAL_CONFIG) --nohome_rc coverage $(BAZEL_CMD_CONFIG) $(BAZEL_INSTRUMENTATION_FILTER) --jobs=HOST_CPUS*0.9 --build_tests_only --test_keep_going=false \
--combined_report=lcov \
bazel $(BAZEL_GLOBAL_CONFIG) --nohome_rc test $(BAZEL_CMD_CONFIG) $(BAZEL_INSTRUMENTATION_FILTER) --jobs=HOST_CPUS*0.9 --build_tests_only --test_keep_going=false \
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe rename the target too

@ti-chi-bot ti-chi-bot Bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 23, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Apr 23, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, Defined2014

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [D3Hunter,Defined2014]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 23, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Apr 23, 2026

[LGTM Timeline notifier]

Timeline:

  • 2026-04-23 04:15:47.664210175 +0000 UTC m=+2225752.869570232: ☑️ agreed by D3Hunter.
  • 2026-04-23 04:18:05.706052981 +0000 UTC m=+2225890.911413028: ☑️ agreed by Defined2014.

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

Labels

approved do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants