feat: Re-enable interface tests and get them succeeding #83

SgtPooki · 2022-11-23T16:39:43Z

Some tests are disabled (temporarily) (19 of them) and will be looked at later with @hacdias, prior to handing project off to Kubo team.

prioritized action items from talk with @hacdias

Look at pubsub, https://github.com/ipfs/interop/blob/master/test/pubsub.js is passing, so we should be able to get them passing here with 0.17
skip refs tests now that we're using 0.17
get CI failures passing
- currently hanging in windows
Open tracking items for 0.17 failing tests (there is already one for refs, need one for each other listed at https://gist.github.com/hacdias/ae85536f8ad258c2c0605efee89173db)
Remove tests that specifically say "not supported by Kubo"
skip tests that use 'proc' or 'js' (if they are failing) and open tracking issue for looking at later by kubo team
- this is already done, tracking issue should consolidate the list of this to make it easier for kubo team to understand that they need looked at. This is being done at ipfs-http-client migration: Additional tasks #56

SgtPooki · 2022-11-23T16:43:05Z

test/interface-tests/src/swarm/peers.js

+      expect(peer).to.have.a.property('muxer')
+      expect(peer).to.have.a.property('streams')


it seems like at some point, these properties were not supposed to be returned if verbose: true was not passed, however, they are being returned.

Also, we probably shouldn't be asserting the lack of a property.. only the existence of properties.

When verbose is not true, Kubo does not return values for them (JSON keys are present, but with empty/default value):

$ ipfs swarm peers --enc=json | jq { "Addr": "/ip4/97.115.80.237/udp/57908/quic", "Peer": "12D3KooWMMnfUUJBzZEA6rv6NyN6QUdJaRk62bc9zECXJNmKYPNP", "Latency": "", "Muxer": "", "Direction": 0, "Streams": null }

so what you want to test here is to ensure they have these empty values when verbose is not passed.

see: https://github.com/ipfs/js-ipfs/blob/6ae5eb7da33adfdee70c7edc09a0f644d5347718/packages/ipfs/test/interface-http-go.js

`ipfs` uses `go-ipfs@0.12.0`, which means the tests we copied will not work the same. See https://github.com/ipfs/js-ipfs/blob/6ae5eb7/packages/ipfs/package.json#L93 Adding multiple go versions in this commit. Will be facilitating the use of these versions in the next commit

some additional tests were skipped, but node tests are succeeding

SgtPooki

ready for review @hacdias. 19 total tests skipped if I counted things correctly.

test/interface-tests.spec.js

package.json

test/interface-tests.spec.js

test/interface-tests/src/files/cp.js

hacdias

Overall, it looks good to me. It's also important to mention that I'm not extremely used to this codebase. Besides the other inlined questions, I would like to understand why there are so many MFS-related tests that were removed (except for mode, and mtime)?

SgtPooki · 2022-12-13T18:20:40Z

Overall, it looks good to me. It's also important to mention that I'm not extremely used to this codebase. Besides the other inlined questions, I would like to understand why there are so many MFS-related tests that were removed (except for mode, and mtime)?

@hacdias this codebase is a complete beast. I think it might be worth getting @achingbrain or @lidel to take a peek, but one major callout is that not all RPC APIs listed at https://docs.ipfs.tech/reference/kubo/rpc/#getting-started are implemented, and some additional tests that are not related to kubo specific RPC calls were supported. i.e. things that were in ipfs-http-client as 'js' or 'proc' but not in kubo.

regarding MFS related tests being removed, the thing to look for is whether the tests being removed were marked as "unsupported" by go-ipfs/kubo in https://github.com/ipfs/js-ipfs/blob/74aee8b3d78f233c3199a3e9a6c0ac628a31a433/packages/ipfs/test/interface-http-go.js. I attempted to leave things that looked like they should be supported when comparing to https://docs.ipfs.tech/reference/kubo/rpc/#getting-started, but there are a ton of tests and I may have missed something.

If some of these MFS tests should be supported by Kubo/go-ipfs, I can re-add them and leave them skipped? this is part of the struggle with pulling ipfs-http-client out. We either need to

strip things and then rebuild the tests (what I recommend now)
or keep the old tests and fix them (what I have been recommending)

After fixing many of these tests, I would highly recommend, and personally prefer #1. The existing tests contain a lot of very odd things that get tests to work in a non-deterministic way and coming at this from an angle of "we need to test X functionality that isn't currently tested" should allow for a much better test than from the angle of "this old test needs modified/updated."

BigLep · 2022-12-13T19:46:39Z

General comment: my understanding is that https://docs.ipfs.tech/reference/kubo/rpc/ is the source of truth of Kubo RPC APIs that we should ideally have tested. (This is because it's generated from the Kubo itself.)

Assuming that is the source of truth, I would expect we can disable any HTTP calls in js-kubo-rpc-client not pertaining to that surface area.

Any places where the Kubo RPC API is not covered by previous (ipfs-http-client) code can be left uncovered for now (and ideally have backlog items).

Using MFS specifically, it's probably worth tracing out:

What HTTP paths are those tests hitting?
Why does that not show up in https://docs.ipfs.tech/reference/kubo/rpc/ ? Is it it intentional? If that isn't a supported API, then I think it's safe to remove the test.

But yes, I know there is a lot more intricacy here than I'm aware of, and it may be helpful if @lidel has any other color.

hacdias · 2022-12-14T12:30:59Z

I agree that someone else should also take a look before we merge, since it's such a massive PR.

Regarding the tests, I would also go with option 1.

lidel

Thank you @SgtPooki and everyone involved, this was a Herculean effort ❤️

Overall lgtm, removals make sense: now that we are Kubo-only, we dont need to duplicate go test and sharness, all we need to test here is if HTTP RPC executes the same way CLI would.

I left some comments / questions, but nothing blocking – feel free to extract any folllow-up tasks related to my comments to separate issues (or as a checkbox in #56)

lidel · 2022-12-14T13:58:41Z

.aegir.js

      return {
        server,
        echoServer,
        pinningService,
        env: {
+          NODE_OPTIONS: '--no-experimental-fetch',


Q: when we run Node with this, does it impact the runtime of tests, or is it run in a separate process?

(if it is the same process, it is a problem: we should be running tests with default Node LTS and no special parameters)

I'm not sure I fully understand your question, but I'll give some context around this:

nodeJS 18 (not being tested currently for node tests in ipfs-http-client, so scope creep for this task) adopted native fetch, see https://nodejs.org/en/blog/announcements/v18-release-announce/#fetch-experimental.

This was previously under an opt-in flag --experimental-fetch, but in 18 was switched to opt-out with --no-experimental-fetch. Basically, any discrepancies between node-fetch and undici's fetch will need to be handled if we don't override global fetch in node with node-fetch.

I have a hunch that some of these discrepancies were one of the problems I had with unblocking the test/node/agent.js test.

When we run tests, the flow is:

node run test

which runs aegir test

which kicks off execa(subprocess) for either c8 or mocha

c8 or mocha: https://github.com/ipfs/aegir/blob/7c5be7fc1690ca0054e3b4dcd0d99c5f083e25d1/src/test/node.js#L21

env passed to the subprocess: https://github.com/ipfs/aegir/blob/7c5be7fc1690ca0054e3b4dcd0d99c5f083e25d1/src/test/node.js#L77

I think item 2 above tells us the answer to your question is that this is a separate process.

lidel · 2022-12-14T14:04:05Z

.github/workflows/js-test-and-release.yml

    steps:
      - uses: actions/checkout@v3
      - uses: actions/setup-node@v3
        with:
-          node-version: ${{ matrix.node }}
+          node-version: lts/*


⚠️

This project is managed via github.com/protocol/.github (config).

Does this mean the changes to this file from this PR will be overwritten by https://github.com/protocol/.github/blob/master/templates/.github/workflows/js-test-and-release.yml the next time CI sync runs?

I believe it will be overwritten:

when repo is added to unified CI (this repo has already been)
https://github.com/protocol/.github/blob/30836503d140fec6454ef6f49d96372a9167757f/.github/actions/add-unified-ci-to-new-go-repos/src/main.ts

copy workflow from .github repo: https://github.com/protocol/.github/blob/b1a2633be4742ee57b26f27ef475d5227b585476/.github/workflows/copy-workflow.yml

code where unified CI compares existing workflows to templated workflows in .github repo: https://github.com/protocol/.github/blob/b1a2633be4742ee57b26f27ef475d5227b585476/.github/workflows/copy-workflow.yml#L115-L124

@galargh can you confirm?

Yes, it's going to get overwritten. Whenever https://github.com/protocol/.github/actions/workflows/dispatch.yml runs, it updates uCI files in all the repositories where it's configured (if needed of course). That's why files managed by uCI have this header:

js-kubo-rpc-client/.github/workflows/js-test-and-release.yml

Lines 1 to 2 in b3da286

# File managed by web3-bot. DO NOT EDIT.

# See https://github.com/protocol/.github/ for details.

Here's an automated PR that wants to update the workflow here: #85 (it would have been automerged if only test-node on windows passed).

If there's a change we want to introduce that would benefit all JS repos participating in uCI, then this would be the place to do it: https://github.com/protocol/.github/blob/master/templates/.github/workflows/js-test-and-release.yml

Side note: the way Unified CI provisions files is different from how GitHub Management does it. For example, if we were to modify https://github.com/ipfs/js-kubo-rpc-client/blob/master/.github/workflows/stale.yml which is managed here https://github.com/ipfs/github-mgmt/blob/eed32757b57354659c07d738d36ecfbc25a033ef/github/ipfs.yml#L4589-L4591, then the next sync would have noticed that the two diverged and would update the content in the YAML config.

So if a project needs to disable node LTS or other node versions for some reason, what is the best way to accomplish that?

Can we make the unified CI workflow dynamic with a default to LTS? I'm not sure the best way to accomplish this but it would allow individual projects to update/override when necessary. Without being overwritten.

src/bitswap/index.js

src/bootstrap/index.js

test/interface-tests/src/dag/import.js

test/interface-tests/src/files/cp.js

lidel · 2022-12-14T16:21:18Z

test/interface-tests/src/swarm/peers.js

+      expect(peer).to.have.a.property('muxer')
+      expect(peer).to.have.a.property('streams')


When verbose is not true, Kubo does not return values for them (JSON keys are present, but with empty/default value):

$ ipfs swarm peers --enc=json | jq { "Addr": "/ip4/97.115.80.237/udp/57908/quic", "Peer": "12D3KooWMMnfUUJBzZEA6rv6NyN6QUdJaRk62bc9zECXJNmKYPNP", "Latency": "", "Muxer": "", "Direction": 0, "Streams": null }

so what you want to test here is to ensure they have these empty values when verbose is not passed.

test/node/agent.js

test/node/request-api.js

…istently

## [2.0.0](v1.0.3...v2.0.0) (2022-12-15) ### ⚠ BREAKING CHANGES * Re-enable interface tests and get them succeeding (#83) ### Features * Re-enable interface tests and get them succeeding ([#83](#83)) ([2819080](2819080)), closes [/github.com/ipfs/js-ipfs/blob/6ae5eb7/packages/ipfs/package.json#L93](https://github.com/ipfs//github.com/ipfs/js-ipfs/blob/6ae5eb7/packages/ipfs/package.json/issues/L93) [/github.com//pull/83#issuecomment-1347820355](https://github.com/ipfs//github.com/ipfs/js-kubo-rpc-client/pull/83/issues/issuecomment-1347820355) ### Trivial Changes * move interface tests back to original name ([56c5c28](56c5c28))

github-actions · 2022-12-15T23:34:08Z

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

SgtPooki linked an issue Nov 23, 2022 that may be closed by this pull request

ipfs-http-client migration: Additional tasks #56

Open

17 tasks

SgtPooki commented Nov 23, 2022

View reviewed changes

SgtPooki added 21 commits November 29, 2022 10:35

chore: sync with ipfs/test/interface-http-go.js

3bf928e

see: https://github.com/ipfs/js-ipfs/blob/6ae5eb7da33adfdee70c7edc09a0f644d5347718/packages/ipfs/test/interface-http-go.js

fix: .bitswap.wantlist before test

58243e5

fix: .bitswap.wantlistForPeer before

8ce3abb

chore: enable running interface tests against multiple kubo versions

27db517

fix: bitswap ipfs spawn types

2a282dc

fix: dht utils ipfs api usage

0c84c21

fix: dag.get test

5b9c92d

fix: dht tests

c8b95e7

fix: .id test

aa3948d

fix: pinning service tests

f8598e1

fix: misc tests

b9e5325

fix: pinning tests

0c182ff

fix: .ping tests

aa1c7e6

fix: .swarm.peers passes

63916fe

fix: .swarm.addrs passes

b6f7c01

fix: .swarm.disconnect passes

e22f89c

fix: all tests are passing in node

bd67b63

some additional tests were skipped, but node tests are succeeding

fix: browser interface tests are passing or skipped

011268f

fix: webworker tests pass

298f534

feat: fix webworker tests

e370e56

SgtPooki force-pushed the 56-investigate-and-document-broken-interface-ipfs-core-tests branch from 7e8c039 to e370e56 Compare November 29, 2022 18:35

SgtPooki marked this pull request as ready for review November 29, 2022 18:37

SgtPooki requested a review from hacdias November 29, 2022 18:37

SgtPooki self-assigned this Nov 29, 2022

SgtPooki commented Nov 29, 2022

View reviewed changes

chore: code cleanup

c002718

SgtPooki changed the title ~~[WIP] feat: Re-enable interface tests and get them succeeding~~ feat: Re-enable interface tests and get them succeeding Nov 29, 2022

hacdias reviewed Dec 13, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

hacdias reviewed Dec 13, 2022

View reviewed changes

test/interface-tests.spec.js Outdated Show resolved Hide resolved

hacdias reviewed Dec 13, 2022

View reviewed changes

test/interface-tests/src/files/cp.js Show resolved Hide resolved

hacdias reviewed Dec 13, 2022

View reviewed changes

chore: remove old go-ipfs version testing support

6fa916a

lidel approved these changes Dec 14, 2022

View reviewed changes

SgtPooki added 6 commits December 15, 2022 12:39

fix(test): node/request-api - trailer headers

7a11168

fix: test running when not in node

e2f3aa0

chore: move byCID function to utils

26c1504

fix(test): 'should import car with roots but no blocks' succeeds cons…

059ce51

…istently

fix(test): re-add 'should export a chunk of a file'

6c27e9d

fix(test): test 'should list peers this node is connected to' specifics

f39eb81

This was referenced Dec 15, 2022

match Kubo RPC bootstrap api endpoints #96

Open

Update config.replace interface test #97

Closed

Fix and re-enable test/node/agent.js concurrency test #98

Open

match Kubo RPC bitswap api endpoints #99

Open

chore: address the rest of the PR comments

70058a2

SgtPooki merged commit 2819080 into master Dec 15, 2022

SgtPooki deleted the 56-investigate-and-document-broken-interface-ipfs-core-tests branch December 15, 2022 22:38

SgtPooki removed the status/in-progress In progress label Dec 15, 2022

github-actions bot added the released label Dec 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Re-enable interface tests and get them succeeding #83

feat: Re-enable interface tests and get them succeeding #83

SgtPooki commented Nov 23, 2022 •

edited

Loading

SgtPooki Nov 23, 2022

lidel Dec 14, 2022

SgtPooki left a comment

hacdias left a comment

SgtPooki commented Dec 13, 2022

BigLep commented Dec 13, 2022

hacdias commented Dec 14, 2022

lidel left a comment •

edited

Loading

lidel Dec 14, 2022

SgtPooki Dec 15, 2022 •

edited

Loading

lidel Dec 14, 2022

SgtPooki Dec 15, 2022

SgtPooki Dec 15, 2022

galargh Dec 16, 2022

SgtPooki Dec 16, 2022

lidel Dec 14, 2022

github-actions bot commented Dec 15, 2022

		expect(peer).to.have.a.property('muxer')
		expect(peer).to.have.a.property('streams')

	# File managed by web3-bot. DO NOT EDIT.
	# See https://github.com/protocol/.github/ for details.

feat: Re-enable interface tests and get them succeeding #83

feat: Re-enable interface tests and get them succeeding #83

Conversation

SgtPooki commented Nov 23, 2022 • edited Loading

prioritized action items from talk with @hacdias

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SgtPooki left a comment

Choose a reason for hiding this comment

hacdias left a comment

Choose a reason for hiding this comment

SgtPooki commented Dec 13, 2022

BigLep commented Dec 13, 2022

hacdias commented Dec 14, 2022

lidel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SgtPooki Dec 15, 2022 • edited Loading

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

github-actions bot commented Dec 15, 2022

SgtPooki commented Nov 23, 2022 •

edited

Loading

lidel left a comment •

edited

Loading

SgtPooki Dec 15, 2022 •

edited

Loading