test: Add multidimensional interop test #1615

MarcoPolo · 2023-03-07T22:41:26Z

Implements: https://github.com/libp2p/test-plans/blob/master/multidim-interop/README.md

MarcoPolo · 2023-03-08T23:57:12Z

Future todos:

Figure out why firefox webrtc isn't working
Figure out why webkit tests fail to create a libp2p node

MarcoPolo · 2023-03-09T00:39:16Z

@ckousik I'm seeing a failure with firefox -> webrtc -> rust (Chrome works fine). https://github.com/libp2p/js-libp2p/actions/runs/4369651820/jobs/7643671824#step:5:5535 " localCert.getFingerprints is not a function". Have you seen that before? https://github.com/libp2p/js-libp2p-webrtc/blob/4c8806c6d2a1a8eff48f0e2248203d48bd84c065/src/transport.ts#L215

MarcoPolo · 2023-03-09T02:41:16Z

I don't know why windows is failing. It seems like it's failing in other PRs as well, so probably not related to these changes

ckousik · 2023-03-09T03:14:02Z

@ckousik I'm seeing a failure with firefox -> webrtc -> rust (Chrome works fine). https://github.com/libp2p/js-libp2p/actions/runs/4369651820/jobs/7643671824#step:5:5535 " localCert.getFingerprints is not a function". Have you seen that before? https://github.com/libp2p/js-libp2p-webrtc/blob/4c8806c6d2a1a8eff48f0e2248203d48bd84c065/src/transport.ts#L215

That's odd, since getFingerprints is required in w3c https://w3c.github.io/webrtc-pc/#rtccertificate-interface . Looking into it now.

MarcoPolo · 2023-03-09T20:19:34Z

This ready for review. This could be faster, our build of head takes 10m. I haven't spent any time investigating this, but we could probably speed this up.

p-shahi · 2023-03-28T17:43:22Z

@achingbrain Can we review and get this in this week (meant to follow up on this in triage today)?

achingbrain

LGTM.

We should add the interop folder to the list of files that get linted to keep everything consistent. At the moment there are some rules violations but we can fix them when they actually start to break something.

Adding 10m to the build time seems a bit excessive - can it be faster?

MarcoPolo · 2023-03-31T17:11:25Z

Per sync discussion:

@achingbrain is hesistant to merge this while it takes >10min. If I bring it down to <10min, we'll merge.

MarcoPolo · 2023-04-07T19:59:59Z

Alright, I spent a good bit of time optimizing this and the tests run in 15 min. I think this is as good as we can get it without reworking the test runner itself. (unfortunately docker run startup time is the limiting factor, with each test taking about 2s to run).

This could improve a bit if we make the rust images smaller, but I wouldn't expect too much.

@achingbrain can I get another approval?

thomaseizinger · 2023-05-05T06:25:11Z

This could improve a bit if we make the rust images smaller, but I wouldn't expect too much.

Tracking this here: libp2p/rust-libp2p#3881

MarcoPolo · 2023-05-16T23:34:17Z

All tests pass! @achingbrain any objections to merging?

thomaseizinger · 2023-05-17T08:39:49Z

@MarcoPolo Do I understand correctly that with this PR js-libp2p will use the same structure of just pointing to a commit-hash in the test-plans repo?

Much in favor of adopting that!

.github/workflows/interop-test.yml

interop/Dockerfile

MarcoPolo · 2023-05-17T17:48:39Z

Do I understand correctly that with this PR js-libp2p will use the same structure of just pointing to a commit-hash in the test-plans repo?

Yes

MarcoPolo · 2023-05-17T22:39:41Z

@achingbrain this is blocking more interop tests in test plans and webrtc interop tests. Since this is only test code, I'll assume your previous approval is still valid unless you say otherwise within the next ~24h.

interop/test/ping.spec.ts

achingbrain · 2023-05-18T16:39:31Z

interop/test/ping.spec.ts

+    const IP = process.env.ip ?? '0.0.0.0'
+    const timeoutSecs: string = process.env.test_timeout_secs ?? '180'
+
+    const options: Libp2pOptions<{ ping: PingService }> = {


Can all this config be built in a separate file and invoked in a beforeEach block?

It's very verbose and doesn't seem like it's part of the test.

I don't see the benefit of having a separate file, but I can move to a beforeEach.

Presumably these tests will test more than just ping in the future, at that point we're going to want to set up a libp2p node in more than one place, hence the external file.

We can make that refactor when we need it, but also this is setup code and not test code so it shouldn't be in the it function.

This is testing whether two libp2p nodes can communicate. A future test may look similar, but I don’t think it makes sense to refactor until we have that future test.

interop/test/ping.spec.ts

achingbrain · 2023-05-18T16:45:05Z

interop/test/ping.spec.ts

+    const node = await createLibp2p(options)
+
+    try {
+      if (isDialer) {


Conditional logic in tests is always raises an eyebrow as you can end up not testing what you think you are testing.

Can this be split into two separate tests?

For example the dialer test could skip itself if the node isn't the dialer on this run.

Why? Every other implementation does it this way.

Looking at the other implementations, none of them are written using a test framework so I don't think they are an example to follow here.

Maybe we shouldn’t use the test framework? I only used it because it was convenient. Happy to not use it.

Do you feel strongly enough against the test framework as to request changes before merging?

I don’t think we should deviate from what the four other implementations do just follow the convention on this test framework, so I would suggest keeping the conditional as is, and , if requested, moving off the test framework.

I would be against splitting this into two tests.

I think using a test framework to write tests is the right thing to do, as is integrating with the existing tooling of a project because it makes the code instantly familiar to other developers which lowers maintenance burden so I think this is the right decision.

I think regardless of framework tests have a certain structure (setup, exercise, verify, teardown), duplication should be minimised and that conditional logic in tests means that later down the line when something goes wrong the author will have to think about which branch is being executed which is a code smell and should be avoided.

I also think that the first test is the most important since people will use it as a starting point for subsequent tests and am somewhat confused as to why you are pushing back on this so hard.

interop/package.json

MarcoPolo · 2023-05-23T17:51:05Z

I've split up the test @achingbrain, but what's annoying now is that one or the other test will always be skipped but it will still spin up and shutdown a libp2p node because of beforeEach and afterEach. Any suggestions? We might be able to replace it(... with it.skip(... dynamically and thus skip the test at the higher level, but that seems a bit hacky.

maschad · 2023-05-24T00:56:47Z

I've split up the test @achingbrain, but what's annoying now is that one or the other test will always be skipped but it will still spin up and shutdown a libp2p node because of beforeEach and afterEach. Any suggestions? We might be able to replace it(... with it.skip(... dynamically and thus skip the test at the higher level, but that seems a bit hacky.

I agree @MarcoPolo it seems to get a bit messy going that route. What do you think about splitting the tests into two separate directories i.e. one for dialler tests and one for listener tests (albeit they are both just one test each at the moment) and we can use just use an npm script like

"test": if [ \"$is_dialer\" = \"true\" ]; then aegir test -t node -f \".dist/test/dialer/*.{node,spec}.js\"; else aegir test -t node -f \".dist/test/listener/*.{node,spec}.js\"; fi

I know we have been going back and forth on this for a while so if you and @achingbrain agree then I could make these changes so that we can land this.

MarcoPolo · 2023-05-24T00:59:22Z

I've split up the test @achingbrain, but what's annoying now is that one or the other test will always be skipped but it will still spin up and shutdown a libp2p node because of beforeEach and afterEach. Any suggestions? We might be able to replace it(... with it.skip(... dynamically and thus skip the test at the higher level, but that seems a bit hacky.

I agree @MarcoPolo it seems to get a bit messy going that route. What do you think about splitting the tests into two separate directories i.e. one for dialler tests and one for listener tests (albeit they are both just one test each at the moment) and we can use just use an npm script like
"test": if [ \"$is_dialer\" = \"true\" ]; then aegir test -t node -f \".dist/test/dialer/*.{node,spec}.js\"; else aegir test -t node -f \".dist/test/listener/*.{node,spec}.js\"; fi

I'm strongly against separate files for this test. I also don't like using bash scripting as the way to check which test to run. Why are we introducing bash into this when we're already working with TypeScript?

maschad · 2023-05-24T01:37:04Z

I'm strongly against separate files for this test.

why is that? As it is now they are two separate tests in one file, I think that having them in separate files improves the modularity and reporting (as you wouldn't see the logs of the test being skipped)

I also don't like using bash scripting as the way to check which test to run. Why are we introducing bash into this when we're already working with TypeScript?

I can appreciate that but I think they serve two different purposes in this case. I would say it's more common to see bash scripting accessing environment variables than Typescript. I would also say it's more intuitive than the dynamic way of skipping tests

Regardless, I wouldn't let either of those concerns block this from being landed and I see that tests pass now 🚀. We can always optimize in a follow up PR 😄

MarcoPolo · 2023-05-24T01:41:46Z

why is that? As it is now they are two separate tests in one file, I think that having them in separate files improves the modularity and reporting (as you wouldn't see the logs of the test being skipped)

Having more files here doesn't improve modularity, it adds indirection. You may as well run the test command and grep for only the tests you want to run.

Is it common to see Bash scripting used in npm test? I've done it a couple of times, but only as a last resort.

maschad · 2023-05-24T02:00:47Z

You may as well run the test command and grep for only the tests you want to run.

That's essentially what the bash script is doing by using the -f flag. The same reasoning applies to how we run the interop tests which are in the interop directory

Having more files here doesn't improve modularity, it adds indirection

I had envisioned that going forward the tests would be split along when the node is dialling and when the node is listening. Like:

| ── dialler
| └── ping.test.ts
|── listener
└── ping.test.ts

As it stands if we add more tests using the current structure we would see something like:

$ npm run test

> libp2p@0.45.0 test
> mocha

 ping tests
    ✓ Test case 1 (passed)
    - Test case 2 (skipped)
    ✓ Test case 3 (passed)
    - Test case 4 (skipped)

  2 passing (12ms)
  2 pending

when debugging as opposed to just seeing the relevant tests for when JS is a dialler or listener.

Is it common to see Bash scripting used in npm test? I've done it a couple of times, but only as a last resort.

Maybe less so within an npm script itself but certainly on CI.

MarcoPolo · 2023-05-24T16:46:20Z

You may as well run the test command and grep for only the tests you want to run.

Sorry this wasn't clear. I was using this an example of what you wouldn't want to do.

p-shahi · 2023-05-25T23:27:27Z

Poking in here: were the discussions mostly resolved i.e. is this guy almost there to getting merged?

achingbrain · 2023-05-26T17:33:24Z

I meant something like:

it('should be a listener', function () {
  if (is_dialer) {
    return this.skip()
  }

  // listener test code here
})

The dynamic skip:

(isDialer ? it.skip : it)('should listen for ping', async () => {

is reminiscent of what we did for the js-ipfs interface tests. It worked, though typing it all was a little hairy.

I think one file for the ping test is ok, definitely don't want to be debugging bash scripts to figure out why tests aren't running.

I think we should get this in and we can finesse it in follow PRs.

MarcoPolo changed the title ~~Initial attempt at multidim CI test~~ feat: Initial attempt at multidim CI test Mar 7, 2023

MarcoPolo marked this pull request as ready for review March 9, 2023 20:14

p-shahi linked an issue Mar 10, 2023 that may be closed by this pull request

Add in-repo testing for js-libp2p libp2p/test-plans#135

Closed

MarcoPolo requested a review from achingbrain March 13, 2023 17:19

achingbrain changed the title ~~feat: Initial attempt at multidim CI test~~ test: Initial attempt at multidim CI test Mar 31, 2023

achingbrain approved these changes Mar 31, 2023

View reviewed changes

MarcoPolo mentioned this pull request Mar 31, 2023

Cache final images instead of build layers libp2p/test-plans#161

Closed

MarcoPolo force-pushed the marco/interop-test branch from 458d016 to 41d3d96 Compare April 7, 2023 17:36

achingbrain mentioned this pull request Apr 21, 2023

Consider libp2p mono repo #824

Closed

MarcoPolo mentioned this pull request May 1, 2023

feat(interop): add js-libp2p v0.44 libp2p/test-plans#171

Merged

Add libp2p-ping-interop test

d4b000d

MarcoPolo force-pushed the marco/interop-test branch from f293f90 to d4b000d Compare May 1, 2023 21:23

MarcoPolo mentioned this pull request May 2, 2023

deps(yamux): update yamux to v0.12 libp2p/rust-libp2p#3013

Merged

4 tasks

Update interop test

f9f3bf0

maschad self-assigned this May 9, 2023

MarcoPolo added 3 commits May 11, 2023 15:21

Merge branch 'master' into marco/interop-test

705f0ee

Update interop deps

aa447b7

Use master interop runner

545eb68

maschad reviewed May 17, 2023

View reviewed changes

.github/workflows/interop-test.yml Outdated Show resolved Hide resolved

interop/Dockerfile Outdated Show resolved Hide resolved

Don't deny any dials in interop test

09b5ff7

Merge branch 'master' into marco/interop-test

4d0fe28

achingbrain requested changes May 18, 2023

View reviewed changes

MarcoPolo added 2 commits May 18, 2023 12:33

PR nits

c851378

Lint

3d6ac52

MarcoPolo requested a review from achingbrain May 18, 2023 20:49

MarcoPolo changed the title ~~test: Initial attempt at multidim CI test~~ feat: Add multidimensional interop test May 18, 2023

MarcoPolo changed the title ~~feat: Add multidimensional interop test~~ test: Add multidimensional interop test May 18, 2023

MarcoPolo mentioned this pull request May 18, 2023

Browser <-> Browser over webrtc-private-to-private libp2p/test-plans#152

Closed

maschad removed their assignment May 23, 2023

Split into two tests

1f2b89e

Dynamically skip test

09c74ab

Set isDialer earlier

76df717

maschad approved these changes May 24, 2023

View reviewed changes

Merge branch 'master' into marco/interop-test

3c52888

achingbrain approved these changes May 26, 2023

View reviewed changes

achingbrain merged commit e92f427 into master May 26, 2023

achingbrain deleted the marco/interop-test branch May 26, 2023 17:34

p-shahi mentioned this pull request Jun 13, 2023

[tracking issue] Interop test-plans for all existing/developing libp2p transports libp2p/test-plans#61

Open

28 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Add multidimensional interop test #1615

test: Add multidimensional interop test #1615

MarcoPolo commented Mar 7, 2023 •

edited

Loading

MarcoPolo commented Mar 8, 2023 •

edited

Loading

MarcoPolo commented Mar 9, 2023

MarcoPolo commented Mar 9, 2023

ckousik commented Mar 9, 2023

MarcoPolo commented Mar 9, 2023

p-shahi commented Mar 28, 2023

achingbrain left a comment

MarcoPolo commented Mar 31, 2023

MarcoPolo commented Apr 7, 2023

thomaseizinger commented May 5, 2023

MarcoPolo commented May 16, 2023

thomaseizinger commented May 17, 2023

MarcoPolo commented May 17, 2023

MarcoPolo commented May 17, 2023

achingbrain May 18, 2023

MarcoPolo May 18, 2023

achingbrain May 19, 2023

MarcoPolo May 19, 2023

achingbrain May 18, 2023

MarcoPolo May 18, 2023

achingbrain May 19, 2023

MarcoPolo May 19, 2023

MarcoPolo May 19, 2023

achingbrain May 22, 2023 •

edited

Loading

MarcoPolo commented May 23, 2023

maschad commented May 24, 2023

MarcoPolo commented May 24, 2023

maschad commented May 24, 2023

MarcoPolo commented May 24, 2023

maschad commented May 24, 2023

MarcoPolo commented May 24, 2023

p-shahi commented May 25, 2023

achingbrain commented May 26, 2023

test: Add multidimensional interop test #1615

test: Add multidimensional interop test #1615

Conversation

MarcoPolo commented Mar 7, 2023 • edited Loading

MarcoPolo commented Mar 8, 2023 • edited Loading

MarcoPolo commented Mar 9, 2023

MarcoPolo commented Mar 9, 2023

ckousik commented Mar 9, 2023

MarcoPolo commented Mar 9, 2023

p-shahi commented Mar 28, 2023

achingbrain left a comment

Choose a reason for hiding this comment

MarcoPolo commented Mar 31, 2023

MarcoPolo commented Apr 7, 2023

thomaseizinger commented May 5, 2023

MarcoPolo commented May 16, 2023

thomaseizinger commented May 17, 2023

MarcoPolo commented May 17, 2023

MarcoPolo commented May 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achingbrain May 22, 2023 • edited Loading

Choose a reason for hiding this comment

MarcoPolo commented May 23, 2023

maschad commented May 24, 2023

MarcoPolo commented May 24, 2023

maschad commented May 24, 2023

MarcoPolo commented May 24, 2023

maschad commented May 24, 2023

MarcoPolo commented May 24, 2023

p-shahi commented May 25, 2023

achingbrain commented May 26, 2023

MarcoPolo commented Mar 7, 2023 •

edited

Loading

MarcoPolo commented Mar 8, 2023 •

edited

Loading

achingbrain May 22, 2023 •

edited

Loading