workloadrepo: Add unit testing for the Workload Repository. #58266

wddevries · 2024-12-15T05:51:22Z

Issue Number: ref #58247

Problem Summary:

This pull request adds unit testing for the Workload Repository code. It also fixes a few issues and refactors some code. Specific changes include:

Create the etcd snap id key the first actual value instead of zero.
Enable snapshot collection for the TIDB_INDEX_USAGE.
Add checks for nil to a few places in the partition handling code.
Partition validation code will only check that partitions cover today and tomorrow.
It will only generate new partitions if no partition covers the current day and the next day.
Validate that the value assigned to tidb_workload_repository_dest is either an empty string or 'table.'
Fix a couple of bugs in the housing keeping process.
Factor out fillInTableNames(), createPartition() and dropOldPartition() for testing purposes.
Pass the current time into createAllTables() and createPartition() so that testing can be deterministic
Reread the information schema in createAllTables() before running code to create missing partitions.
Add unit tests for the Workload Repository.

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-12-15T05:51:39Z

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

pkg/util/workloadrepo/worker_test.go

bb7133 · 2024-12-16T03:22:46Z

Please resolve the conflicts.

codecov · 2024-12-18T00:35:28Z

Codecov Report

Attention: Patch coverage is 76.47059% with 44 lines in your changes missing coverage. Please review.

Project coverage is 73.6320%. Comparing base (67249cb) to head (bf04e11).
Report is 10 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #58266        +/-   ##
================================================
+ Coverage   73.0858%   73.6320%   +0.5462%     
================================================
  Files          1676       1676                
  Lines        463685     465641      +1956     
================================================
+ Hits         338888     342861      +3973     
+ Misses       103941     101924      -2017     
  Partials      20856      20856

Flag	Coverage Δ
integration	`42.8211% <7.4866%> (?)`
unit	`72.3003% <75.9358%> (+0.0200%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`45.7419% <ø> (+0.0209%)`	⬆️

henrybw

Pretty much LGTM

pkg/util/workloadrepo/worker.go

pkg/util/workloadrepo/utils.go

pkg/util/workloadrepo/worker_test.go

xhebox

I don't have much objections on these little refactors, LGTM

xhebox

�[31m�[1mTIMEOUT: �[0m//pkg/util/workloadrepo:workloadrepo_test (Summary)
      /home/jenkins/.tidb/tmp/63a9840cd0739f2c243bb46478607469/execroot/__main__/bazel-out/k8-fastbuild/testlogs/pkg/util/workloadrepo/workloadrepo_test/test.log
      /home/jenkins/.tidb/tmp/63a9840cd0739f2c243bb46478607469/execroot/__main__/bazel-out/k8-fastbuild/testlogs/pkg/util/workloadrepo/workloadrepo_test/test_attempts/attempt_1.log
      /home/jenkins/.tidb/tmp/63a9840cd0739f2c243bb46478607469/execroot/__main__/bazel-out/k8-fastbuild/testlogs/pkg/util/workloadrepo/workloadrepo_test/test_attempts/attempt_2.log
�[31m�[1mERROR: �[0m/home/jenkins/agent/workspace/pingcap/tidb/ghpr_unit_test/tidb/pkg/util/workloadrepo/BUILD.bazel:41:8: Testing //pkg/util/workloadrepo:workloadrepo_test failed: Test failed, aborting
==================== Test output for //pkg/util/workloadrepo:workloadrepo_test:

Is there a problem for the test?

tiprow · 2024-12-31T01:10:49Z

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

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

wddevries · 2025-01-01T00:26:45Z

/test unit-test

tiprow · 2025-01-01T00:27:12Z

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

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

henrybw

LGTM

pkg/util/workloadrepo/utils.go

ti-chi-bot · 2025-01-06T22:16:49Z

@henrybw: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

LGTM

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.

…pdated for snapshot command changes.)

… partitions when no partition covers tomorrow.

…er an empty string or 'table.'

…() for testing purposes.

…t testing can be deterministic.

…AllPartitions().

…a struct.

…workers at initialization.

xhebox · 2025-01-08T03:30:58Z

pkg/util/workloadrepo/worker_test.go

 	if !testWorker {
+		workerCtx.etcdClient = nil


Hmm, can we do workCtx = worker{}? GO will init every field to zero.

I was under the impression that multiple unit tests were being run within the same process and that the global variables were not reinitialized between tests, but my experimentation seems to indicate this is not the case. I will fix it.

bb7133

LGTM

ti-chi-bot · 2025-01-09T03:03:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bb7133, henrybw, xhebox

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

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 · 2025-01-09T03:03:59Z

[LGTM Timeline notifier]

Timeline:

2025-01-08 03:33:28.277255836 +0000 UTC m=+324551.566087541: ☑️ agreed by xhebox.
2025-01-09 03:03:58.62722449 +0000 UTC m=+409181.916056197: ☑️ agreed by bb7133.

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 Dec 15, 2024

xhebox reviewed Dec 15, 2024

View reviewed changes

pkg/util/workloadrepo/worker_test.go Show resolved Hide resolved

wddevries force-pushed the add_unit_testing branch from 6409e21 to c77178b Compare December 18, 2024 02:50

henrybw reviewed Dec 21, 2024

View reviewed changes

xhebox reviewed Dec 26, 2024

View reviewed changes

wddevries force-pushed the add_unit_testing branch from c77178b to 0206059 Compare December 28, 2024 06:47

xhebox reviewed Dec 30, 2024

View reviewed changes

henrybw approved these changes Jan 6, 2025

View reviewed changes

pkg/util/workloadrepo/utils.go Outdated Show resolved Hide resolved

wddevries force-pushed the add_unit_testing branch 2 times, most recently from 5b44f73 to 3da6355 Compare January 7, 2025 09:06

wddevries added 12 commits January 7, 2025 18:11

Create the etcd snap id key with actual new value instead of zero. (U…

7b16fb9

…pdated for snapshot command changes.)

Enable snapshot collection for the TIDB_INDEX_USAGE table.

80bd51b

Add checks for nil to a few places in the partition handling code.

c3612e4

Only check that for partitions that cover tomorrow, and only generate…

9d91ddb

… partitions when no partition covers tomorrow.

Validate that value assigned to tidb_workload_repository_dest is eith…

2ad6bc1

…er an empty string or 'table.'

Fix a couple of bugs in housing keeping.

f0a544b

Factor out fillInTableNames(), createPartition() and dropOldPartition…

36fb3f7

…() for testing purposes.

Pass current time into the createAllTables and createPartition so tha…

af1fd63

…t testing can be deterministic.

Reread the infoschema after creating the tables before running create…

b85a9b9

…AllPartitions().

Add tests for Workload Repository.

d483521

Fix CI issues.

a40575f

Fix CI issues 2.

9bbd77f

wddevries added 11 commits January 7, 2025 18:27

Fix CI issues 3.

c4f8d1e

Enabled snapshot for TIDB_STATEMENTS_STATS.

bcd183c

Check that there are actually partitions if GetPartitionInfo returns …

789d7ce

…a struct.

Rework partition generation code to be simplier.

6a93a23

Fix some spellings.

bc8fb8a

Add the workload tables list to the worker struct for testing purposes.

44cc9b8

Set timeout for tests to long.

23414ae

Clear the state of workerCtx between test.

4e04988

Don't use the workerCtx object in test were it is unneeded.

106dd44

Fix potential deadlock by creating Tickers for snapshot and sampling …

8de60a6

…workers at initialization.

Fix typo in comment.

5aa4be0

wddevries force-pushed the add_unit_testing branch from 3da6355 to 5aa4be0 Compare January 8, 2025 03:02

xhebox approved these changes Jan 8, 2025

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 8, 2025

Remove unneeded initiation code from unit tests.

bf04e11

bb7133 approved these changes Jan 9, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 9, 2025

ti-chi-bot bot merged commit 2ae0b0a into pingcap:master Jan 9, 2025
24 checks passed

wddevries deleted the add_unit_testing branch January 9, 2025 03:13

winoros mentioned this pull request Jan 19, 2025

util: split the tests of workloadrepo into multiple shard #59030

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

workloadrepo: Add unit testing for the Workload Repository. #58266

workloadrepo: Add unit testing for the Workload Repository. #58266

wddevries commented Dec 15, 2024

tiprow bot commented Dec 15, 2024

bb7133 commented Dec 16, 2024

codecov bot commented Dec 18, 2024 •

edited

Loading

henrybw left a comment

xhebox left a comment

xhebox left a comment

tiprow bot commented Dec 31, 2024

wddevries commented Jan 1, 2025

tiprow bot commented Jan 1, 2025

henrybw left a comment

ti-chi-bot bot commented Jan 6, 2025

xhebox Jan 8, 2025

wddevries Jan 9, 2025

bb7133 left a comment

ti-chi-bot bot commented Jan 9, 2025

ti-chi-bot bot commented Jan 9, 2025

workloadrepo: Add unit testing for the Workload Repository. #58266

workloadrepo: Add unit testing for the Workload Repository. #58266

Conversation

wddevries commented Dec 15, 2024

Release note

tiprow bot commented Dec 15, 2024

bb7133 commented Dec 16, 2024

codecov bot commented Dec 18, 2024 • edited Loading

Codecov Report

henrybw left a comment

Choose a reason for hiding this comment

xhebox left a comment

Choose a reason for hiding this comment

xhebox left a comment

Choose a reason for hiding this comment

tiprow bot commented Dec 31, 2024

wddevries commented Jan 1, 2025

tiprow bot commented Jan 1, 2025

henrybw left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jan 6, 2025

xhebox Jan 8, 2025

Choose a reason for hiding this comment

wddevries Jan 9, 2025

Choose a reason for hiding this comment

bb7133 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jan 9, 2025

ti-chi-bot bot commented Jan 9, 2025

[LGTM Timeline notifier]

codecov bot commented Dec 18, 2024 •

edited

Loading