Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

migrate test-infra to testify for br/pkg #27178

Closed
21 tasks done
Tracked by #26022
tisonkun opened this issue Aug 13, 2021 · 10 comments
Closed
21 tasks done
Tracked by #26022

migrate test-infra to testify for br/pkg #27178

tisonkun opened this issue Aug 13, 2021 · 10 comments
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. type/enhancement The issue or PR belongs to an enhancement.

Comments

@tisonkun tisonkun added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Aug 13, 2021
@tisonkun
Copy link
Contributor Author

br is original pingcap/br, we should break down more subtasks.

@YuJuncen
Copy link
Contributor

Some analyses 🤔

General

The most simple way for adapting go check is to use the suite interface of testify: replace the call of c.Assert with corresponding testify method.

Some of "stateless" tests, which:

  1. Has a suite of type struct{}
  2. Defined nop lifecycle methods.

...can be refactored as plain go test cases.

Suites

The lifecycle methods of gocheck can be adapted via:

testify gocheck
SetupSuite SetUpSuite
SetupTest SetUpTest
TearDownTest TearDownTest
TearDownSuite TearDownSuite

Checkers

The following Checker are used by BR:

BytesEquals // can be replaced with require.Equal
DeepEquals // same as above
Equals // same as above
// Reason see https://github.com/stretchr/testify/blob/acba37e5db06f0093b465a7d47822bf13644b66c/assert/assertions.go#L58-L76 

ErrorMatches // require.EqualError
FieldEquals // custom checker, using `zapcore.Field.Equals`
FitsTypeOf // maybe require.IsType
Greater // require.Greater
GreaterEqual // require.GreaterOrEqual
HasKey // ?
HasLen // require.Len
IsFalse // require.False
IsNil // require.Nil
IsTrue // require.True
Less // require.Less
LessEqual // require.LessOrEqual
Matches // require.Regexp?
Not // testify didn't support `Checker`, let alone combinators over them...
NotNil // require.NotNil
PanicMatches // maybe require.PanicsWith{Error,Object}?
RangeEquals // custom checker, checking the start key and end keys are equal.
isAbout // custom checker, checking whether two floating numbers are near enough.
jsonEquals // require.JSONEq
posEq // custom checker, checking the pos of a parser.
unblocksBetween // custom checker, checks whether a wait group done in a time interval. 
versionEquals // custom checker for semvers.

Some of the checkers has alternative methods in Assertions from testify. For custom checkers, we may make functions like posEq(t *testing.T, <arg list of checker>, interface{}... msgAndArgs), and delegate the check to methods in require.

Commentf(...)

Each assertions from testify accepts a sequence of msgAndArgs, the items in Commentf can be placed here.

the possibility of auto migration

Given a normal test file here, we can migrate it from gocheck to testify via:

  1. Check the test suite in the file(unexpectedly, some files may contains more than one or no suites).
  2. if it is a "stateless" suite, do:
    a. remove the test suite.
    b. remove the accepter of test functions bound to this suite.
    c. replace the argument c *C with t *testing.T.
    d. replace calling to c.Assert with corresponding method in testify.
    e. if (d) meets a checker cannot be converted to method in testify, leave a hole here and hint the user, like:
// TODO impl the custom checker
TODO_posEq(t, ...)
  1. if it is a "stateful" suite, do:
    a. compose a testify.Suite in to the test suite.
    b. rename the lifecycle methods bound to the suite.
    (accessing to the c might would be fixed manually.)
    c. do the same thing for assertions as stateless test cases.

These steps (maybe?) are deterministic, which means it's possible to write a script with go/ast and go/parser to automate them. 🤔

Or we can also do these things manually, as a refinement or inspection of our test cases.

@tisonkun
Copy link
Contributor Author

The most simple way for adapting go check is to use the suite interface of testify

@YuJuncen No. We don't do that. Please see also #26082 (comment) and this Chinese post for details. And #27747 for an example.

@tisonkun
Copy link
Contributor Author

tisonkun commented Sep 17, 2021

the possibility of auto migration

@YuJuncen @feitian124 already creates a tool you may make use of to automatically migrate #26022 (comment) . However, I'd prefer just do it manually with some "global find and replace" for common pattern.

If you try to do something automatically and correctly, you're always in trouble - you're now find a new, and probably huge, question to resolve, which is unnecessary to the original problem.

@tisonkun
Copy link
Contributor Author

Commentf(...)

Just use require.Equalf and require.Greaterf.

@tisonkun
Copy link
Contributor Author

Also, it is not a 1 to 1 mapping as c.Assert(err, IsNil) is almost require.NoError(t, err), but c.Assert(sth, IsNil) is require.Nil(t, sth). We don't push the migration to one man or two, as you see; we run it with join force and for br topic the current problem is to find a proper separation: possibly in packages or files.

If you can find such one, I'm glad to update the description and create subtasks.

@tisonkun tisonkun changed the title migrate test-infra to testify for br pkg migrate test-infra to testify for br/pkg pkg Sep 17, 2021
@tisonkun
Copy link
Contributor Author

The first thing I found is that these tests are all under br/pkg

@tisonkun tisonkun changed the title migrate test-infra to testify for br/pkg pkg migrate test-infra to testify for br/pkg Sep 17, 2021
@YuJuncen
Copy link
Contributor

YuJuncen commented Sep 17, 2021

@tisonkun
For suites: Well, if suite isn't acceptable, maybe TestMain can help to set up a test.

需要 Setup/TearDown Suite 式的逻辑的时候,可以通过手动装配测试与子测试来达成。

func TestTeardownParallel(t *testing.T) {
    // This Run will not return until the parallel tests finish.
    t.Run("group", func(t *testing.T) {
        t.Run("Test1", parallelTest1)
        t.Run("Test2", parallelTest2)
        t.Run("Test3", parallelTest3)
    })
    // <tear-down code>
}

Sounds good, but sometimes when setting up, some variables in the suite also have been set.

Any idea better than global variables or closures? Well, yet another suite implementation.

func TestTeardownParallel(t *testing.T) {
    suite := setupSuite()
    t.Run("group", func(t *testing.T) {
        t.Run("Test1", suite.parallelTest1)
        t.Run("Test2", suite.parallelTest2)
        t.Run("Test3", suite.parallelTest3)
    })
    // <tear-down code>
}

For automating: Maybe refactor tools like eg can help?

@tisonkun
Copy link
Contributor Author

tisonkun commented Sep 18, 2021

For automating: Maybe refactor tools like eg can help?

@YuJuncen You're welcome to try on a subtask. And if it works well, I hope you can write it down to teach others how to achieve it.

@tisonkun tisonkun added the good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. label Sep 21, 2021
@tisonkun tisonkun added the type/enhancement The issue or PR belongs to an enhancement. label Feb 10, 2022
@tisonkun
Copy link
Contributor Author

Closed as all subtasks done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
2 participants