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

Move TCL test unit/tls to Go case #1012

Merged
merged 22 commits into from
Oct 20, 2022
Merged

Conversation

PragmaTwice
Copy link
Member

It closes #995.

tisonkun and others added 4 commits October 19, 2022 11:43
Signed-off-by: tison <wander4096@gmail.com>
cmd *exec.Cmd

addr *net.TCPAddr
tlsAddr *net.TCPAddr
Copy link
Member Author

@PragmaTwice PragmaTwice Oct 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A TLS-enabled server has both a non-TLS port and a TLS port which can work at the same time, so I think we need two fields here.

@PragmaTwice PragmaTwice marked this pull request as ready for review October 19, 2022 14:18
PragmaTwice and others added 6 commits October 19, 2022 22:56
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I push a few commits and notice that TLS tests may work on darwin now (work for me locally, I suspect it's about TLS server name). Turn on to verify.

@tisonkun
Copy link
Member

Emmm...Unfortunately it doesn't. Reverting...

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Member

--- FAIL: TestBitmap (403.30s)
    --- FAIL: TestBitmap/SETBIT/GETBIT/BITCOUNT/BITPOS_boundary_check_(type_bitmap) (15.33s)
        bitmap_test.go:146: 
            	Error Trace:	/Users/runner/work/incubator-kvrocks/incubator-kvrocks/tests/gocase/unit/type/bitmap/bitmap_test.go:146
            	Error:      	Not equal: 
            	            	expected: int(1)
            	            	actual  : int64(0)
            	Test:       	TestBitmap/SETBIT/GETBIT/BITCOUNT/BITPOS_boundary_check_(type_bitmap)

@PragmaTwice @git-hulk I don't know why it keeps failing...Cannot reproduce locally and should not be related to this patch (although I try to "fix" it, it fails continuously).

@git-hulk
Copy link
Member

git-hulk commented Oct 20, 2022

ow why it keeps failing...Cannot reproduce locally and should not be related to this patch (although I try to "fix" it, it fail

Yes, not related to current PR, I'm wondering if it's caused by Go parallel run tests.

@tisonkun
Copy link
Member

@git-hulk No. Golang only runs tests in parallel if you write t.Parallel().

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Member

--- FAIL: TestBitmap (403.30s)
    --- FAIL: TestBitmap/SETBIT/GETBIT/BITCOUNT/BITPOS_boundary_check_(type_bitmap) (15.33s)
        bitmap_test.go:146: 
            	Error Trace:	/Users/runner/work/incubator-kvrocks/incubator-kvrocks/tests/gocase/unit/type/bitmap/bitmap_test.go:146
            	Error:      	Not equal: 
            	            	expected: int(1)
            	            	actual  : int64(0)
            	Test:       	TestBitmap/SETBIT/GETBIT/BITCOUNT/BITPOS_boundary_check_(type_bitmap)

@PragmaTwice @git-hulk I don't know why it keeps failing...Cannot reproduce locally and should not be related to this patch (although I try to "fix" it, it fails continuously).

@vmihailenco do you have any ideas here? This test is our last test to be migrated with go-redis XD

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Member

It seems that the bitmap failure does related to this patch. I'm digging into the test scripts.

See identical CI runs: https://github.com/apache/incubator-kvrocks/actions/runs/3287762862/jobs/5417324374

@vmihailenco
Copy link

@vmihailenco do you have any ideas here?

Perhaps try maxOffset/8 + 1?

@tisonkun
Copy link
Member

@vmihailenco Let me try it if this run still fails. It's strange since it happens only for darwin platform.

Co-authored-by: Twice <twice.mliu@gmail.com>
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending for merging...

@tisonkun
Copy link
Member

After I add back -bench=. in x.py, tests passed. Really weird >_<

@tisonkun tisonkun merged commit ce33994 into apache:unstable Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move TCL test unit/tls to Go case
4 participants