*: force forbiding to use `mock.NewContext` in production code #56654

lcwangchao · 2024-10-15T07:56:20Z

What problem does this PR solve?

Issue Number: close #53388

What changed and how does it work?

Add checks to avoid to use mock.NewContext in production code.

Move the function mock.NewContext to a standalone file which has a //go:build !codes header to avoid to build this function in production code. So if some one uses mock.NewContext in some production code, make server will fail.
Introduce a function mock.NewContextDeprecated and add a // Deprecated comment for it. This method should only be used in some legacy code.
Remove mock.NewContext usage in other production codes.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
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

Release note

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

None

tiprow · 2024-10-15T07:56:55Z

Hi @lcwangchao. 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.

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-sigs/prow repository.

codecov · 2024-10-15T08:18:39Z

Codecov Report

Attention: Patch coverage is 77.41935% with 7 lines in your changes missing coverage. Please review.

Project coverage is 57.5422%. Comparing base (e020f6a) to head (066dee6).
Report is 33 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #56654         +/-   ##
=================================================
- Coverage   73.3452%   57.5422%   -15.8030%     
=================================================
  Files          1628       1779        +151     
  Lines        449555     645030     +195475     
=================================================
+ Hits         329727     371165      +41438     
- Misses        99598     248838     +149240     
- Partials      20230      25027       +4797

Flag	Coverage Δ
integration	`39.6996% <58.0645%> (?)`
unit	`72.9072% <77.4193%> (+0.4535%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.9478% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`63.1008% <ø> (+17.4930%)`	⬆️

lcwangchao · 2024-10-15T08:21:25Z

pkg/util/sqlexec/mock/mock.go

 // RestrictedSQLExecutorKey is the key to represent MockRestrictedSQLExecutorMockRecorder in ctx.
 type RestrictedSQLExecutorKey struct{}

 // String implements the string.Stringer interface.
 func (k RestrictedSQLExecutorKey) String() string {
 	return "__MockRestrictedSQLExecutor"
 }
-
-// WrapAsSCtx wraps the MockRestrictedSQLExecutor into sessionctx.Context.
-func WrapAsSCtx(exec *MockRestrictedSQLExecutor) sessionctx.Context {


Moved to https://github.com/pingcap/tidb/pull/56654/files#diff-8fb9fe4df83ff8e7b3a5a94cd53e7bf8a679f7a6e8b31104b581cc34f9b22d19R46-R52

lcwangchao · 2024-10-15T08:24:56Z

/retest

tiprow · 2024-10-15T08:25:17Z

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

In response to this:

/retest

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-sigs/prow repository.

YangKeao

LGTM

lcwangchao · 2024-10-15T10:31:22Z

/retest

tiprow · 2024-10-15T10:31:45Z

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

In response to this:

/retest

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-sigs/prow repository.

lcwangchao · 2024-10-15T11:59:58Z

/retest

tiprow · 2024-10-15T12:00:19Z

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

In response to this:

/retest

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-sigs/prow repository.

lcwangchao · 2024-10-15T12:06:45Z

/retest

tiprow · 2024-10-15T12:07:07Z

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

In response to this:

/retest

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-sigs/prow repository.

lcwangchao · 2024-10-15T14:29:46Z

/retest

tiprow · 2024-10-15T14:30:09Z

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

In response to this:

/retest

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-sigs/prow repository.

ti-chi-bot · 2024-10-15T16:08:59Z

[LGTM Timeline notifier]

Timeline:

2024-10-15 08:45:24.740238811 +0000 UTC m=+347121.889148643: ☑️ agreed by YangKeao.
2024-10-15 16:08:58.177574808 +0000 UTC m=+373735.326484640: ☑️ agreed by CbcWestwolf.

Makefile

lcwangchao · 2024-10-16T06:02:58Z

/retest

tiprow · 2024-10-16T06:03:21Z

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

In response to this:

/retest

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-sigs/prow repository.

ti-chi-bot · 2024-10-16T13:44:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CbcWestwolf, hawkingrei, tangenta, YangKeao

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [CbcWestwolf,YangKeao,hawkingrei,tangenta]
~~pkg/ddl/OWNERS~~ [tangenta]
~~pkg/planner/OWNERS~~ [hawkingrei]
~~pkg/statistics/OWNERS~~ [hawkingrei]

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

lcwangchao · 2024-10-17T01:31:03Z

/retest

tiprow · 2024-10-17T01:31:24Z

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

In response to this:

/retest

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-sigs/prow repository.

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/planner SIG: Planner labels Oct 15, 2024

lcwangchao mentioned this pull request Oct 15, 2024

remove mock.Context usage in production code #53388

Closed

lcwangchao force-pushed the forbidmockctx branch from b85b0ce to da4dcbc Compare October 15, 2024 07:59

*: add some checks to forbid to use mock.Context in release code

7af9fee

lcwangchao force-pushed the forbidmockctx branch from da4dcbc to 7af9fee Compare October 15, 2024 08:06

lcwangchao changed the title *: add some checks to forbid to use mock.Context in release code *: force to forbid using mock.NewContext in production code Oct 15, 2024

lcwangchao changed the title *: force to forbid using mock.NewContext in production code *: force forbiding to use mock.NewContext in production code Oct 15, 2024

lcwangchao commented Oct 15, 2024

View reviewed changes

YangKeao approved these changes Oct 15, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Oct 15, 2024

fix plugin build

36653d3

lcwangchao force-pushed the forbidmockctx branch from 21d352c to 36653d3 Compare October 15, 2024 11:37

CbcWestwolf approved these changes Oct 15, 2024

View reviewed changes

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

hawkingrei reviewed Oct 16, 2024

View reviewed changes

Makefile Show resolved Hide resolved

upate bazel

066dee6

hawkingrei approved these changes Oct 16, 2024

View reviewed changes

tangenta approved these changes Oct 16, 2024

View reviewed changes

ti-chi-bot bot added the approved label Oct 16, 2024

ti-chi-bot bot merged commit 1052a55 into pingcap:master Oct 17, 2024
24 checks passed

lcwangchao deleted the forbidmockctx branch October 17, 2024 02:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: force forbiding to use `mock.NewContext` in production code #56654

*: force forbiding to use `mock.NewContext` in production code #56654

lcwangchao commented Oct 15, 2024

tiprow bot commented Oct 15, 2024

codecov bot commented Oct 15, 2024 •

edited

Loading

lcwangchao Oct 15, 2024 •

edited

Loading

lcwangchao commented Oct 15, 2024

tiprow bot commented Oct 15, 2024

YangKeao left a comment

lcwangchao commented Oct 15, 2024

tiprow bot commented Oct 15, 2024

lcwangchao commented Oct 15, 2024

tiprow bot commented Oct 15, 2024

lcwangchao commented Oct 15, 2024

tiprow bot commented Oct 15, 2024

lcwangchao commented Oct 15, 2024

tiprow bot commented Oct 15, 2024

ti-chi-bot bot commented Oct 15, 2024

lcwangchao commented Oct 16, 2024

tiprow bot commented Oct 16, 2024

ti-chi-bot bot commented Oct 16, 2024

lcwangchao commented Oct 17, 2024

tiprow bot commented Oct 17, 2024

*: force forbiding to use mock.NewContext in production code #56654

*: force forbiding to use mock.NewContext in production code #56654

Conversation

lcwangchao commented Oct 15, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Oct 15, 2024

codecov bot commented Oct 15, 2024 • edited Loading

Codecov Report

lcwangchao Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

lcwangchao commented Oct 15, 2024

tiprow bot commented Oct 15, 2024

YangKeao left a comment

Choose a reason for hiding this comment

lcwangchao commented Oct 15, 2024

tiprow bot commented Oct 15, 2024

lcwangchao commented Oct 15, 2024

tiprow bot commented Oct 15, 2024

lcwangchao commented Oct 15, 2024

tiprow bot commented Oct 15, 2024

lcwangchao commented Oct 15, 2024

tiprow bot commented Oct 15, 2024

ti-chi-bot bot commented Oct 15, 2024

[LGTM Timeline notifier]

lcwangchao commented Oct 16, 2024

tiprow bot commented Oct 16, 2024

ti-chi-bot bot commented Oct 16, 2024

lcwangchao commented Oct 17, 2024

tiprow bot commented Oct 17, 2024

*: force forbiding to use `mock.NewContext` in production code #56654

*: force forbiding to use `mock.NewContext` in production code #56654

codecov bot commented Oct 15, 2024 •

edited

Loading

lcwangchao Oct 15, 2024 •

edited

Loading