storage: migrate test framework to testify #5139

LLThomas · 2022-06-09T09:44:24Z

Signed-off-by: LLThomas zs033@qq.com

What problem does this PR solve?

Issue Number: Ref #4813

What is changed and how does it work?

As the title says.

Check List

Tests

Unit test

Release note

None

Signed-off-by: LLThomas <zs033@qq.com>

ti-chi-bot · 2022-06-09T09:44:28Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
rleungx

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.

LLThomas · 2022-06-09T09:51:29Z

When I run test, TestLoadMinServiceSafePoint can't pass.
The error occurs in "re.Nil(ssp)".

  // verify that service safe point with ServiceID 0 has been removed
  ssp, err := storage.LoadServiceSafePoint(testKeySpace, "0")
  re.NoError(err)
  re.Nil(ssp)

Then, I test the original code.

  // verify that service safe point with ServiceID 0 has been removed
  ssp, err := storage.LoadServiceSafePoint(testKeySpace, "0")
  c.Assert(err, IsNil)
  c.Assert(ssp, IsNil)

"c.Assert(ssp, IsNil)" also has the same error.
Maybe it is a bug.

codecov · 2022-06-09T09:53:19Z

Codecov Report

Merging #5139 (633e6a7) into master (f82e15e) will increase coverage by 0.10%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5139      +/-   ##
==========================================
+ Coverage   75.46%   75.56%   +0.10%     
==========================================
  Files         309      309              
  Lines       30515    30515              
==========================================
+ Hits        23028    23060      +32     
+ Misses       5474     5450      -24     
+ Partials     2013     2005       -8

Flag	Coverage Δ
unittests	`75.56% <ø> (+0.10%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/tempurl/tempurl.go	`45.00% <0.00%> (-15.00%)`	⬇️
server/storage/storage.go	`64.86% <0.00%> (-5.41%)`	⬇️
server/tso/tso.go	`69.49% <0.00%> (-2.26%)`	⬇️
server/storage/endpoint/meta.go	`61.79% <0.00%> (-2.25%)`	⬇️
server/region_syncer/client.go	`84.32% <0.00%> (-1.50%)`	⬇️
server/storage/hot_region_storage.go	`78.14% <0.00%> (-1.10%)`	⬇️
server/cluster/unsafe_recovery_controller.go	`80.18% <0.00%> (-0.37%)`	⬇️
server/tso/allocator_manager.go	`64.55% <0.00%> (-0.34%)`	⬇️
server/id/id.go	`80.95% <0.00%> (ø)`
server/cluster/coordinator.go	`70.45% <0.00%> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f82e15e...633e6a7. Read the comment docs.

rleungx · 2022-06-09T10:33:14Z

server/storage/hot_region_storage_test.go

@@ -207,7 +198,7 @@ func (t *testHotRegionStorage) TestHotRegionDelete(c *C) {
 	num := 0
 	for next, err := iter.Next(); next != nil && err == nil; next, err = iter.Next() {
 		num++
-		c.Assert(reflect.DeepEqual(next, &historyHotRegions[defaultRemainDay-num]), IsTrue)
+		re.True(reflect.DeepEqual(next, &historyHotRegions[defaultRemainDay-num]))


Equal is enough.

I think "re.True" is more suitable for functions that return bool value.
And I see an example that has been merged.

- c.Assert(rep.GetReplicationStatus().GetDrAutoSync().GetPauseRegionSplit(), IsTrue) + re.True(rep.GetReplicationStatus().GetDrAutoSync().GetPauseRegionSplit())

https://github.com/tikv/pd/pull/5100/files#diff-f36769a34db4e8e3412376a3a7c5be18b2f7f8ab90287fc48c7f4445bc716218L221

See #5104, require.Equal is used to replace reflect.DeepEqual.

Suggested change

re.True(reflect.DeepEqual(next, &historyHotRegions[defaultRemainDay-num]))

re.Equal(&historyHotRegions[defaultRemainDay-num], next)

Oh, I got it, I'll fix it.

JmPotato · 2022-06-09T13:43:49Z

When I run test, TestLoadMinServiceSafePoint can't pass. The error occurs in "re.Nil(ssp)".
  // verify that service safe point with ServiceID 0 has been removed
  ssp, err := storage.LoadServiceSafePoint(testKeySpace, "0")
  re.NoError(err)
  re.Nil(ssp)
Then, I test the original code.
  // verify that service safe point with ServiceID 0 has been removed
  ssp, err := storage.LoadServiceSafePoint(testKeySpace, "0")
  c.Assert(err, IsNil)
  c.Assert(ssp, IsNil)
"c.Assert(ssp, IsNil)" also has the same error. Maybe it is a bug.

This test relies on the failpoint injection, you can see it is enabled on line 107.

pd/server/storage/storage_gc_test.go

Line 107 in 86a6821

    
           c.Assert(failpoint.Enable("github.com/tikv/pd/server/storage/endpoint/removeExpiredKeys", "return(true)"), IsNil)

To make the failpoint work, you should call make failpoint-enable before running the test.

Signed-off-by: LLThomas <zs033@qq.com>

LLThomas · 2022-06-10T02:39:31Z

When I run test, TestLoadMinServiceSafePoint can't pass. The error occurs in "re.Nil(ssp)".
  // verify that service safe point with ServiceID 0 has been removed
  ssp, err := storage.LoadServiceSafePoint(testKeySpace, "0")
  re.NoError(err)
  re.Nil(ssp)
Then, I test the original code.
  // verify that service safe point with ServiceID 0 has been removed
  ssp, err := storage.LoadServiceSafePoint(testKeySpace, "0")
  c.Assert(err, IsNil)
  c.Assert(ssp, IsNil)
"c.Assert(ssp, IsNil)" also has the same error. Maybe it is a bug.
This test relies on the failpoint injection, you can see it is enabled on line 107.

pd/server/storage/storage_gc_test.go

Line 107 in 86a6821

c.Assert(failpoint.Enable("github.com/tikv/pd/server/storage/endpoint/removeExpiredKeys", "return(true)"), IsNil)

To make the failpoint work, you should call make failpoint-enable before running the test.

Thanks! I got it.

JmPotato · 2022-06-10T03:17:19Z

/merge

ti-chi-bot · 2022-06-10T03:17:19Z

@JmPotato: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

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 ti-community-infra/tichi repository.

ti-chi-bot · 2022-06-10T03:17:21Z

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

Commit hash: 633e6a7

ref tikv#4813 As the title says. Signed-off-by: LLThomas <zs033@qq.com>

storage: migrate test framework to testify

1945440

Signed-off-by: LLThomas <zs033@qq.com>

ti-chi-bot added the release-note-none label Jun 9, 2022

ti-chi-bot requested review from disksing and JmPotato June 9, 2022 09:44

rleungx reviewed Jun 9, 2022

View reviewed changes

fix: require.Equal is used to replace reflect.DeepEqual

633e6a7

Signed-off-by: LLThomas <zs033@qq.com>

rleungx approved these changes Jun 10, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 10, 2022

JmPotato approved these changes Jun 10, 2022

View reviewed changes

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 Jun 10, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 10, 2022

ti-chi-bot merged commit ae157b5 into tikv:master Jun 10, 2022

rleungx mentioned this pull request Jun 13, 2022

Tracking Issue for Test Restructure #4813

Closed

85 tasks

CabinfeverB pushed a commit to CabinfeverB/pd that referenced this pull request Jul 14, 2022

storage: migrate test framework to testify (tikv#5139)

5e45e7a

ref tikv#4813 As the title says. Signed-off-by: LLThomas <zs033@qq.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: migrate test framework to testify #5139

storage: migrate test framework to testify #5139

LLThomas commented Jun 9, 2022

ti-chi-bot commented Jun 9, 2022 •

edited

LLThomas commented Jun 9, 2022

codecov bot commented Jun 9, 2022 •

edited

rleungx Jun 9, 2022

LLThomas Jun 9, 2022

JmPotato Jun 9, 2022

LLThomas Jun 10, 2022

JmPotato commented Jun 9, 2022

LLThomas commented Jun 10, 2022

JmPotato commented Jun 10, 2022

ti-chi-bot commented Jun 10, 2022

ti-chi-bot commented Jun 10, 2022

	re.True(reflect.DeepEqual(next, &historyHotRegions[defaultRemainDay-num]))
	re.Equal(&historyHotRegions[defaultRemainDay-num], next)

storage: migrate test framework to testify #5139

storage: migrate test framework to testify #5139

Conversation

LLThomas commented Jun 9, 2022

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Jun 9, 2022 • edited

LLThomas commented Jun 9, 2022

codecov bot commented Jun 9, 2022 • edited

Codecov Report

rleungx Jun 9, 2022

Choose a reason for hiding this comment

LLThomas Jun 9, 2022

Choose a reason for hiding this comment

JmPotato Jun 9, 2022

Choose a reason for hiding this comment

LLThomas Jun 10, 2022

Choose a reason for hiding this comment

JmPotato commented Jun 9, 2022

LLThomas commented Jun 10, 2022

JmPotato commented Jun 10, 2022

ti-chi-bot commented Jun 10, 2022

ti-chi-bot commented Jun 10, 2022

ti-chi-bot commented Jun 9, 2022 •

edited

codecov bot commented Jun 9, 2022 •

edited