expression: replace `mock.Context` with `StaticExprContext` in tests #53007

lcwangchao · 2024-04-30T08:27:22Z

What problem does this PR solve?

Issue Number: close #53006

Problem Summary:

After we introduced some static implements for EvalContext and ExprContext. It's better to replace mock.Context with StaticExprContext for the following reasons:

reduce the dependencies of expression
make the test easier to maintain without relying on a hug context

What changed and how does it work?

mock.NewContext is replaced with mockStmtExprCtx
createContext is replaced with mockStmtTruncateAsWarningExprCtx

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-04-30T08:29:21Z

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/test-infra repository.

pkg/expression/helper.go

codecov · 2024-04-30T08:50:56Z

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 74.5399%. Comparing base (4fe70da) to head (aea6a28).
Report is 38 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #53007        +/-   ##
================================================
+ Coverage   72.4796%   74.5399%   +2.0603%     
================================================
  Files          1493       1493                
  Lines        429362     429402        +40     
================================================
+ Hits         311200     320076      +8876     
+ Misses        98931      89419      -9512     
- Partials      19231      19907       +676

Flag	Coverage Δ
integration	`49.2460% <40.0000%> (?)`
unit	`71.2597% <80.0000%> (-0.1192%)`	⬇️

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

Components	Coverage Δ
dumpling	`53.9957% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`50.4684% <ø> (+9.0282%)`	⬆️

lcwangchao · 2024-04-30T10:19:23Z

/retest

lcwangchao · 2024-05-08T08:33:27Z

pkg/expression/helper.go

-	if tp == mysql.TypeTimestamp || tp == mysql.TypeDatetime || tp == mysql.TypeDate {
-		err = value.ConvertTimeZone(time.Local, ctx.Location())
-		if err != nil {
-			return value, err
-		}
+	if err = value.ConvertTimeZone(defaultTime.Location(), ctx.Location()); err != nil {
+		return value, err


This is the only file that has some logic change:

remove tp == mysql.TypeTimestamp || tp == mysql.TypeDatetime || tp == mysql.TypeDate because it seems useless. I check all the places that call getTimeCurrentTimeStamp. The tp is always time type. Only converting timezone for time types is also meaningless, so I removed it.

Change the first argument of ConvertTimeZone from time.Local to defaultTime.Location() because this the practical meaning for this line. Because defaultTime is always in local time zone if it is get from session, it is also right for previous code. But the StaticEvalContext returns a current time with the same zone of the context, it's better to make a change.

PTAL @AilinKid

How about add a intest.Assert here?

(It's also fine to not add it, because IMO it's intuitive to call getTimeCurrentTimeStamp with time type.

pkg/expression/evaluator_test.go

AilinKid · 2024-05-08T08:48:45Z

pkg/expression/helper.go

-	if tp == mysql.TypeTimestamp || tp == mysql.TypeDatetime || tp == mysql.TypeDate {
-		err = value.ConvertTimeZone(time.Local, ctx.Location())
-		if err != nil {
-			return value, err
-		}
+	if err = value.ConvertTimeZone(defaultTime.Location(), ctx.Location()); err != nil {
+		return value, err


YangKeao

Rest LGTM

YangKeao · 2024-05-17T05:32:25Z

pkg/expression/helper.go

-	if tp == mysql.TypeTimestamp || tp == mysql.TypeDatetime || tp == mysql.TypeDate {
-		err = value.ConvertTimeZone(time.Local, ctx.Location())
-		if err != nil {
-			return value, err
-		}
+	if err = value.ConvertTimeZone(defaultTime.Location(), ctx.Location()); err != nil {
+		return value, err


How about add a intest.Assert here?

(It's also fine to not add it, because IMO it's intuitive to call getTimeCurrentTimeStamp with time type.

ti-chi-bot · 2024-05-17T05:35:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: YangKeao
Once this PR has been reviewed and has the lgtm label, please assign xuhuaiyu for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

~~OWNERS~~ [YangKeao]
pkg/expression/OWNERS

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 · 2024-05-17T05:35:39Z

[LGTM Timeline notifier]

Timeline:

2024-05-17 05:35:39.678529107 +0000 UTC m=+1804293.435664850: ☑️ agreed by YangKeao.

ti-chi-bot bot added release-note-none size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 30, 2024

lcwangchao commented Apr 30, 2024

View reviewed changes

pkg/expression/helper.go Outdated Show resolved Hide resolved

lcwangchao force-pushed the replaceexprtest branch from a815865 to cb81965 Compare April 30, 2024 08:59

lcwangchao force-pushed the replaceexprtest branch from cb81965 to e25a77d Compare May 8, 2024 08:25

lcwangchao commented May 8, 2024

View reviewed changes

AilinKid reviewed May 8, 2024

View reviewed changes

lcwangchao force-pushed the replaceexprtest branch 2 times, most recently from 7d077f7 to b84a362 Compare May 8, 2024 12:02

lcwangchao added 3 commits May 13, 2024 18:41

expression: replace mock.Context with StaticExprContext in tests

042d1f3

update

e6022a9

update

aea6a28

lcwangchao force-pushed the replaceexprtest branch from b84a362 to aea6a28 Compare May 13, 2024 10:41

YangKeao approved these changes May 17, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm label May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: replace `mock.Context` with `StaticExprContext` in tests #53007

expression: replace `mock.Context` with `StaticExprContext` in tests #53007

lcwangchao commented Apr 30, 2024 •

edited

tiprow bot commented Apr 30, 2024

codecov bot commented Apr 30, 2024 •

edited

lcwangchao commented Apr 30, 2024

lcwangchao May 8, 2024

AilinKid May 8, 2024

YangKeao May 17, 2024

AilinKid May 8, 2024

YangKeao left a comment

YangKeao May 17, 2024

ti-chi-bot bot commented May 17, 2024

ti-chi-bot bot commented May 17, 2024

expression: replace mock.Context with StaticExprContext in tests #53007

Are you sure you want to change the base?

expression: replace mock.Context with StaticExprContext in tests #53007

Conversation

lcwangchao commented Apr 30, 2024 • edited

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Apr 30, 2024

codecov bot commented Apr 30, 2024 • edited

Codecov Report

lcwangchao commented Apr 30, 2024

lcwangchao May 8, 2024

Choose a reason for hiding this comment

AilinKid May 8, 2024

Choose a reason for hiding this comment

YangKeao May 17, 2024

Choose a reason for hiding this comment

AilinKid May 8, 2024

Choose a reason for hiding this comment

YangKeao left a comment

Choose a reason for hiding this comment

YangKeao May 17, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented May 17, 2024

ti-chi-bot bot commented May 17, 2024

[LGTM Timeline notifier]

expression: replace `mock.Context` with `StaticExprContext` in tests #53007

expression: replace `mock.Context` with `StaticExprContext` in tests #53007

lcwangchao commented Apr 30, 2024 •

edited

codecov bot commented Apr 30, 2024 •

edited