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

CI: Fast multidimensional Interop tests #1991

Merged
merged 8 commits into from
Jan 13, 2023
Merged

Conversation

MarcoPolo
Copy link
Collaborator

@MarcoPolo MarcoPolo commented Jan 11, 2023

Runs the current branch against other supported versions. Tests all transport, muxer, and security combinations. Tests dialing and listening. Runs in <10 min compared to the existing interop test of ~30 min (but effectively ~15min since there are two tests that run in parallel).

In total this currently runs 95 integration tests compared to the 2 testground tests. To be fair the testground test run a test where all different nodes are on the same network and try to ping each other. That would be equivalent to roughly 5-15 tests here.

Draft until the corresponding test-plans PR is merged: libp2p/test-plans#99

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Nice work! Mostly nits here.

I'm wondering if we can use Docker tags instead of capturing the container ID, that might simplify the build process a tiny bit.

.github/workflows/interop-test.yml Outdated Show resolved Hide resolved
.github/workflows/interop-test.yml Show resolved Hide resolved
.github/workflows/interop-test.yml Show resolved Hide resolved
path: ./test-plans/ping-image.tar
run-multidim-interop:
needs: build-ping-container
uses: "libp2p/test-plans/.github/workflows/run-testplans.yml@marco/wo-testground-opts"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use master?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This needs the branch that's in review. It will be master once that's merged.

test-plans/cmd/ping/main.go Outdated Show resolved Hide resolved
test-plans/cmd/ping/main.go Outdated Show resolved Hide resolved
test-plans/cmd/ping/main.go Outdated Show resolved Hide resolved
lukechampine.com/blake3 v1.1.7 // indirect
)

replace github.com/libp2p/go-libp2p => ../
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be a separate module? These replace statements have proven to be incredibly painful in the past (when you update a dependency, you also have to run go mod tidy here, otherwise CI fill fail your build).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doesn't have to be a separate module. Although if this isn't a separate module, then the test dependencies bubble up to the main module. So Redis would show up as a dependency to go-libp2p, even though it's only used for this test.

Unless, I'm wrong about that. In which case I'll happily not have this be a separate module.

Copy link
Contributor

Choose a reason for hiding this comment

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

So Redis would show up as a dependency to go-libp2p, even though it's only used for this test.

Yeah, let's avoid that.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can (later) add a pre-commit git hook that checks that the go.mod here is up to date.

test-plans/ping-versions.template.json Outdated Show resolved Hide resolved
test-plans/PingDockerfile Outdated Show resolved Hide resolved
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

LGTM. Please merge the test-plans PR first, and update the ref to master here.

test-plans/Makefile Show resolved Hide resolved
test-plans/cmd/ping/main.go Outdated Show resolved Hide resolved
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.

2 participants