tests: Use `tk.MustHavePlan` and `tk.MustNotHavePlan` instead of `tk.HasPlan` by Defined2014 · Pull Request #46947 · pingcap/tidb

Defined2014 · 2023-09-13T09:44:54Z

What problem does this PR solve?

Issue Number: close #46945

Problem Summary:

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

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

Documentation

Release note

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

None

tiprow · 2023-09-13T09:45:19Z

Hi @Defined2014. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2023-09-13T10:06:45Z

Codecov Report

Merging #46947 (2be2004) into master (62771ed) will decrease coverage by 0.5408%.
Report is 15 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #46947        +/-   ##
================================================
- Coverage   73.2424%   72.7016%   -0.5408%     
================================================
  Files          1332       1356        +24     
  Lines        397659     404014      +6355     
================================================
+ Hits         291255     293725      +2470     
- Misses        87750      91644      +3894     
+ Partials      18654      18645         -9

Flag	Coverage Δ
integration	`28.6856% <ø> (?)`
unit	`73.2411% <ø> (-0.0013%)`	⬇️

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

Components	Coverage Δ
dumpling	`53.9913% <ø> (-0.0532%)`	⬇️
parser	`84.9771% <ø> (-0.0033%)`	⬇️
br	`48.5539% <ø> (-4.3603%)`	⬇️

Defined2014 · 2023-09-14T01:42:12Z

/test check-dev

tiprow · 2023-09-14T01:42:41Z

@Defined2014: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/test check-dev

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mjonss

Suggestion to change the function name.

With 'Must' it changes the English grammar, so 'Must Have' is the correct form.

testkit/testkit.go

mjonss

LGTM with some suggestions and questions. Feel free to ignore them, since the PR is improving the situation anyway.

executor/partition_table_test.go

executor/union_scan_test.go

mjonss · 2023-09-14T20:18:29Z

planner/core/integration_test.go

-		{
-			// 3 table scans
-			"a < 2 or a < 10 or a > 11", []string{"1", "100"},
-		},
-		{
-			// 3 index scans
-			"c < 10 or c < 11 or c > 50", []string{"1", "10", "100"},
-		},


Should these return "TableFullScan" or something else? Maybe the cases struct could be extended with matching MustHavePlan/MustNotHavePlan?

Yes, it's TableFullScan. Not sure it should be checked in this test or not.

planner/core/integration_test.go

table/tables/cache_test.go

testkit/testkit.go

Defined2014 · 2023-09-15T03:56:38Z

/test unit-test

tiprow · 2023-09-15T03:57:12Z

@Defined2014: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/test unit-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

bb7133

LGTM

ti-chi-bot · 2023-09-15T04:00:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bb7133, mjonss

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~~ [bb7133,mjonss]

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 · 2023-09-15T04:00:10Z

[LGTM Timeline notifier]

Timeline:

2023-09-14 20:23:46.519506329 +0000 UTC m=+203392.487094379: ☑️ agreed by mjonss.
2023-09-15 04:00:09.295637082 +0000 UTC m=+230775.263225132: ☑️ agreed by bb7133.

tiprow · 2023-09-15T04:15:33Z

@Defined2014: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
tiprow_fast_test	`6156296`	link	true	`/test tiprow_fast_test`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Defined2014 · 2023-09-15T04:50:02Z

/test check-dev

tiprow · 2023-09-15T04:50:34Z

@Defined2014: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/test check-dev

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Defined2014 added 6 commits September 13, 2023 17:01

update

5bdf1ab

update

7730606

update

76660af

update

efa0ed5

upate

13411bd

fix fmt

66707e5

Defined2014 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 13, 2023

ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 13, 2023

Defined2014 added 2 commits September 13, 2023 17:51

update

7411130

update

9943444

Defined2014 added 7 commits September 13, 2023 18:07

update

4b79dca

update

d8128e0

Merge branch 'master' into fix-46945

0c91248

update

775687e

update

263b85a

update

ce862ca

upate

84656d9

Defined2014 added 3 commits September 14, 2023 09:55

update

43c8d5e

update

fee548e

Merge branch 'master' into fix-46945

7570274

Defined2014 changed the title ~~[WIP] tests: Use tk.MustHasPlan and tk.MustNotHasPlan instead of tk.HasPlan~~ tests: Use tk.MustHasPlan and tk.MustNotHasPlan instead of tk.HasPlan Sep 14, 2023

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 14, 2023

Defined2014 requested a review from winoros September 14, 2023 09:15

Defined2014 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2023

Defined2014 requested review from bb7133 and hawkingrei September 14, 2023 09:15

mjonss reviewed Sep 14, 2023

View reviewed changes

testkit/testkit.go Outdated Show resolved Hide resolved

testkit/testkit.go Outdated Show resolved Hide resolved

update

d69a1ab

Defined2014 changed the title ~~tests: Use tk.MustHasPlan and tk.MustNotHasPlan instead of tk.HasPlan~~ tests: Use tk.MustHavePlan and tk.MustNotHavePlan instead of tk.HasPlan Sep 14, 2023

update

e82fff4

Defined2014 requested a review from xhebox September 14, 2023 13:20

mjonss approved these changes Sep 14, 2023

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Sep 14, 2023

Defined2014 added 2 commits September 15, 2023 10:45

update

87a53cc

update

6156296

bb7133 approved these changes Sep 15, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 15, 2023

Defined2014 added 4 commits September 15, 2023 12:32

update

82d35f8

follow comments

216655f

update

0c3a56c

update

2be2004

ti-chi-bot bot merged commit 4c1f28b into pingcap:master Sep 15, 2023

Defined2014 deleted the fix-46945 branch September 15, 2023 05:17

Conversation

Defined2014 commented Sep 13, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

tiprow bot commented Sep 13, 2023

Uh oh!

codecov bot commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Defined2014 commented Sep 14, 2023

Uh oh!

tiprow bot commented Sep 14, 2023

Uh oh!

mjonss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mjonss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mjonss Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

Defined2014 Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Defined2014 commented Sep 15, 2023

Uh oh!

tiprow bot commented Sep 15, 2023

Uh oh!

bb7133 left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Sep 15, 2023

Uh oh!

ti-chi-bot bot commented Sep 15, 2023

[LGTM Timeline notifier]

Uh oh!

tiprow bot commented Sep 15, 2023

Uh oh!

Defined2014 commented Sep 15, 2023

Uh oh!

tiprow bot commented Sep 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Sep 13, 2023 •

edited

Loading