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

rpc,internal/cmdtest: increase timeout in tests #27083

Merged
merged 3 commits into from
May 22, 2023

Conversation

jsvisa
Copy link
Contributor

@jsvisa jsvisa commented Apr 11, 2023

I'm running the tests in my dev machine(m1 mac air), and some test case failed

I ran the test cases on my local machine (macOS M1 Air) and found that some test cases could not be passed.

run with command:

go test ./cmd/geth/... -v 

test case 1

    test_cmd.go:241: killing the child process (timeout)
    test_cmd.go:115: not enough output, got until ◊:
        ---------------- (stdout text)
        
        ---------------- (expected text)
        ◊undefined
        
--- FAIL: TestUnlockFlagPasswordFile (5.03s)
=== RUN   TestUnlockFlagPasswordFileWrongPassword

test case 2

=== NAME  TestAttachWithHeaders
    test_cmd.go:261: (stderr:42) Fatal: Failed to start the JavaScript console: Post "http://localhost:58843": context deadline exceeded
--- PASS: TestRemoteDbWithHeaders (10.14s)
=== NAME  TestAttachWithHeaders
    attach_test.go:81: Test fail, expected invocation to succeed
--- FAIL: TestAttachWithHeaders (10.14s)
FAIL

test case ...

After checking, I discovered that this was due to the timeout being set too short.

@jsvisa jsvisa requested review from fjl and holiman as code owners April 11, 2023 02:19
@SecureCodeSolutionsDev
Copy link

It seems like the test cases are failing due to a timeout issue. The timeout value might be too short for some of the test cases to complete successfully. You can try increasing the timeout value and running the test cases again to see if they pass.

To do this, you can use the -timeout flag with the go test command. For example, to set the timeout value to 10 seconds, you can run the following command:

go test ./cmd/geth/... -v -timeout 10s

You can adjust the timeout value as needed to ensure that the test cases have enough time to complete successfully.

@jsvisa
Copy link
Contributor Author

jsvisa commented Apr 14, 2023

It seems like the test cases are failing due to a timeout issue. The timeout value might be too short for some of the test cases to complete successfully. You can try increasing the timeout value and running the test cases again to see if they pass.

To do this, you can use the -timeout flag with the go test command. For example, to set the timeout value to 10 seconds, you can run the following command:


go test ./cmd/geth/... -v -timeout 10s

You can adjust the timeout value as needed to ensure that the test cases have enough time to complete successfully.

Thanks for the advice, I'll have a try

@@ -43,7 +43,7 @@ var (
const (
// Timeouts
defaultDialTimeout = 10 * time.Second // used if context has no deadline
subscribeTimeout = 5 * time.Second // overall timeout eth_subscribe, rpc_modules calls
subscribeTimeout = 10 * time.Second // overall timeout eth_subscribe, rpc_modules calls
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks like it not only concerns tests, but actually modifies production code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, BTW, it's strange that the dial timeout is set to 10 seconds, but the subscribe timeout is only set to 5 seconds. So I set it to the same as dial's.

@jsvisa
Copy link
Contributor Author

jsvisa commented Apr 17, 2023

It seems like the test cases are failing due to a timeout issue. The timeout value might be too short for some of the test cases to complete successfully. You can try increasing the timeout value and running the test cases again to see if they pass.
To do this, you can use the -timeout flag with the go test command. For example, to set the timeout value to 10 seconds, you can run the following command:


go test ./cmd/geth/... -v -timeout 10s

You can adjust the timeout value as needed to ensure that the test cases have enough time to complete successfully.

Thanks for the advice, I'll have a try

@SecureCodeSolutionsDev thanks for the advice, but it can't work

@Dawidowak

This comment was marked as off-topic.

@Dawidowak

This comment was marked as off-topic.

@Dawidowak

This comment was marked as off-topic.

@holiman
Copy link
Contributor

holiman commented May 9, 2023

A problem with this change is that it bumps TestAttachWelcome from 5s to 30s. However, that can be fixed by not waiting, just killing it once the test is done:

diff --git a/cmd/geth/consolecmd_test.go b/cmd/geth/consolecmd_test.go
index 46bdf3c90d..5046906c0a 100644
--- a/cmd/geth/consolecmd_test.go
+++ b/cmd/geth/consolecmd_test.go
@@ -116,7 +116,7 @@ func TestAttachWelcome(t *testing.T) {
 		waitForEndpoint(t, endpoint, 3*time.Second)
 		testAttachWelcome(t, geth, endpoint, httpAPIs)
 	})
-	geth.ExpectExit()
+	geth.Kill()
 }
 
 func testAttachWelcome(t *testing.T, geth *testgeth, endpoint, apis string) {

@jsvisa
Copy link
Contributor Author

jsvisa commented May 9, 2023

A problem with this change is that it bumps TestAttachWelcome from 5s to 30s. However, that can be fixed by not waiting, just killing it once the test is done:

diff --git a/cmd/geth/consolecmd_test.go b/cmd/geth/consolecmd_test.go
index 46bdf3c90d..5046906c0a 100644
--- a/cmd/geth/consolecmd_test.go
+++ b/cmd/geth/consolecmd_test.go
@@ -116,7 +116,7 @@ func TestAttachWelcome(t *testing.T) {
 		waitForEndpoint(t, endpoint, 3*time.Second)
 		testAttachWelcome(t, geth, endpoint, httpAPIs)
 	})
-	geth.ExpectExit()
+	geth.Kill()
 }
 
 func testAttachWelcome(t *testing.T, geth *testgeth, endpoint, apis string) {

thanks, just curious how could you find which test cases were affected?

@holiman
Copy link
Contributor

holiman commented May 22, 2023

thanks, just curious how could you find which test cases were affected?

Sorry, missed this question. IIRC I think I added a panic, and checked which tests triggered it.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM. The TestCustomGenesis often fails on windows appveyor nowadays, possibly due to the kzg addition. This should fix it.

@holiman holiman changed the title rpc,internal/cmdtest: increase timeout to ensure test pass in macOS m1 rpc,internal/cmdtest: increase timeout in tests May 22, 2023
@holiman holiman added this to the 1.11.7 milestone May 22, 2023
@holiman holiman merged commit 6fe0252 into ethereum:master May 22, 2023
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
This change gives the cmd-tests have a bit more time to finish before getting forcibly torn down.
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
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.

4 participants