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.

Sign up for GitHub

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

cmd/devp2p/internal/ethtest: skip large tx test on github build #28794

Merged
merged 6 commits into from
Jan 12, 2024
Merged

cmd/devp2p/internal/ethtest: skip large tx test on github build #28794

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dab41e9
devp2p/internal: skip large tx test on 32-bit builds
s1na Jan 11, 2024
6905647
rm blank line
s1na Jan 11, 2024
8211e01
skip on short flag or 386 arch
s1na Jan 12, 2024
0f7f6b8
add skip reason
s1na Jan 12, 2024
c1fd670
fix typo
s1na Jan 12, 2024
fdf653d
only short
s1na Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions cmd/devp2p/internal/ethtest/suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package ethtest
import (
"crypto/rand"
"math/big"
"math/bits"
"reflect"

"github.com/ethereum/go-ethereum/common"
Expand Down Expand Up @@ -62,7 +63,7 @@ func NewSuite(dest *enode.Node, chainDir, engineURL, jwt string) (*Suite, error)
}

func (s *Suite) EthTests() []utesting.Test {
return []utesting.Test{
tests := []utesting.Test{
// status
{Name: "TestStatus", Fn: s.TestStatus},
// get block headers
Expand All @@ -78,10 +79,14 @@ func (s *Suite) EthTests() []utesting.Test {
// test transactions
{Name: "TestTransaction", Fn: s.TestTransaction},
{Name: "TestInvalidTxs", Fn: s.TestInvalidTxs},
{Name: "TestLargeTxRequest", Fn: s.TestLargeTxRequest},
{Name: "TestNewPooledTxs", Fn: s.TestNewPooledTxs},
{Name: "TestBlobViolations", Fn: s.TestBlobViolations},
}
// This test is often failing on 32-bit CI.
if bits.UintSize > 32 {
tests = append(tests, utesting.Test{Name: "TestLargeTxRequest", Fn: s.TestLargeTxRequest})
}
return tests
Copy link
Contributor

@fjl fjl Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would prefer if we skip the test in the actual Go unit test that runs this instead of excluding it from the list here. The tests are executed here:

go-ethereum/cmd/devp2p/internal/ethtest/suite_test.go

Line 66 in 2e8b118

result := utesting.RunTests([]utesting.Test{{Name: test.Name, Fn: test.Fn}}, os.Stdout)

So please add a condition there, like

if bits.UintSize == 32 && test.Name == "TestLargeTxRequest" {
    t.Skip()
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to use different semantics. Instead of
"Are we running on 32-bit? If so, skip", the logic should be
"Are we running with the tag short? If so, skip".

Copy link
Contributor

Choose a reason for hiding this comment

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

I.e

	if testing.Short() && test.Name == "TestLargeTxRequest"{
		t.Skip("skipped in short-mode")
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please take another look. I refactored so skipping is based both on short tag or 386 arch. This is similar to how we decide on skipping the state tests:

go-ethereum/tests/init_test.go

Lines 144 to 149 in ae4ea04

if testing.Short() {
return "skipped in -short mode", false
}
if isWin32 {
return "skipped on 32bit windows", false
}

}

func (s *Suite) SnapTests() []utesting.Test {
Expand Down
Loading