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 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 57.0801%. Comparing base (479f4be) to head (de97549).
Report is 1086 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #53007         +/-   ##
=================================================
- Coverage   72.8176%   57.0801%   -15.7375%     
=================================================
  Files          1524       1649        +125     
  Lines        435793     631414     +195621     
=================================================
+ Hits         317334     360412      +43078     
- Misses        98816     247146     +148330     
- Partials      19643      23856       +4213

Flag	Coverage Δ
integration	`37.0358% <14.2857%> (?)`
unit	`71.7889% <71.4285%> (-0.0259%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.9656% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`52.5507% <ø> (+6.4191%)`	⬆️

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:39Z

[LGTM Timeline notifier]

Timeline:

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

ti-chi-bot · 2024-06-28T06:03:33Z

[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 windtalker 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:

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

lcwangchao · 2024-06-28T09:57:34Z

/retest

tiprow · 2024-06-28T09:57:56Z

@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-07-05T22:17:01Z

PR needs rebase.

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.

ti-chi-bot · 2024-11-18T06:20:51Z

@lcwangchao: 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
pull-integration-e2e-test	`de97549`	link	true	`/test pull-integration-e2e-test`

Full PR test history. Your PR dashboard.

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.

ti-chi-bot bot added 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 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 3 times, most recently 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 Indicates a PR needs 1 more LGTM. label May 17, 2024

lcwangchao added 3 commits May 21, 2024 10:00

expression: replace mock.Context with StaticExprContext in tests

13b7c60

update

4552986

update

0b566fa

lcwangchao force-pushed the replaceexprtest branch from aea6a28 to 0b566fa Compare May 21, 2024 02:00

lcwangchao force-pushed the replaceexprtest branch 3 times, most recently from 1cff68e to 45eab99 Compare June 28, 2024 09:29

Merge branch 'master' into replaceexprtest

de97549

lcwangchao force-pushed the replaceexprtest branch from 45eab99 to de97549 Compare June 28, 2024 09:36

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 5, 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 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

tiprow bot commented Apr 30, 2024

Uh oh!

Uh oh!

codecov bot commented Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lcwangchao commented Apr 30, 2024

Uh oh!

lcwangchao May 8, 2024

Choose a reason for hiding this comment

Uh oh!

AilinKid May 8, 2024

Choose a reason for hiding this comment

Uh oh!

YangKeao May 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AilinKid May 8, 2024

Choose a reason for hiding this comment

Uh oh!

YangKeao left a comment

Choose a reason for hiding this comment

Uh oh!

YangKeao May 17, 2024

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented May 17, 2024

[LGTM Timeline notifier]

Uh oh!

ti-chi-bot bot commented Jun 28, 2024

Uh oh!

lcwangchao commented Jun 28, 2024

Uh oh!

tiprow bot commented Jun 28, 2024

Uh oh!

ti-chi-bot bot commented Jul 5, 2024

Uh oh!

ti-chi-bot bot commented Nov 18, 2024

Uh oh!

Uh oh!

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

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

lcwangchao commented Apr 30, 2024 •

edited

Loading

codecov bot commented Apr 30, 2024 •

edited

Loading