Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

topsql: add more test to check resource tag for each RPC request #33623

Merged
merged 14 commits into from Apr 7, 2022
Merged

topsql: add more test to check resource tag for each RPC request #33623

merged 14 commits into from Apr 7, 2022

Conversation

crazycs520
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #33208

Problem Summary: Add more test to check resource tag for each RPC request

What is changed and how it works?

Check List

Tests

  • Integration test

Release note

None

@crazycs520
topsql: add more test for check set resource tag
cfa732d 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
tiny refine
0da9e7e 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Mar 31, 2022 •
edited

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • breeswish
  • zhongzc

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note-none size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 31, 2022
@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
fix load data doesn't set resource tag
e44c4de 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
refine
e75b8d6 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
@crazycs520
Copy link
Contributor Author

/run-all-tests

@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 1, 2022
@crazycs520
fix test
fd170da 
Signed-off-by: crazycs520 <crazycs520@gmail.com>
crazycs520
crazycs520 commented Apr 1, 2022
View reviewed changes
server/main_test.go
// Enable TopSQL for all test, and check the resource tag for each RPC request.
// This is used to detect which codes are not tracked by TopSQL.
topsqlstate.EnableTopSQL()
unistore.CheckResourceTagForTopSQLInGoTest = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@breeswish Now, all test case in server pkg will check the resource tag now.

I try to enable this test check in other pkg, but will meet many error, so currently I only enable this test in server pkg.

Copy link
Member

Choose a reason for hiding this comment

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

but will meet many error

Does these error comes from we are not tagging the request correctly? If so, maybe we need to add tracking issues to track them and fix them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I will create issue to track this. and some error are cause by test itself.

@sre-bot
Copy link
Contributor

sre-bot commented Apr 2, 2022 •
edited

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/514b7a5c794e508a74532863138345dccdd2a7ec

zhongzc
zhongzc reviewed Apr 2, 2022
View reviewed changes
ddl/delete_range.go Outdated Show resolved Hide resolved
zhongzc
zhongzc reviewed Apr 2, 2022
View reviewed changes
ddl/util/util.go Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 2, 2022
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 6, 2022
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2022
@crazycs520
Copy link
Contributor Author

/run-all-tests

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 7, 2022
@crazycs520
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 514b7a5

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 7, 2022
@ti-chi-bot ti-chi-bot merged commit e9b7f77 into pingcap:master Apr 7, 2022
@crazycs520 crazycs520 deleted the topsql-test6 branch April 7, 2022 05:07
@crazycs520 crazycs520 mentioned this pull request Apr 13, 2022
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breezewish breezewish breezewish approved these changes

@zhongzc zhongzc zhongzc approved these changes

Assignees
No one assigned
Labels
component/test release-note-none size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add more test for Top SQL
5 participants
@crazycs520 @ti-chi-bot @sre-bot @breezewish @zhongzc