-
Notifications
You must be signed in to change notification settings - Fork 1.2k
chore: run interop tests as part of the build #2440
Conversation
can you use https://github.com/hugomrdias/connect-deps instead of npm link please ? |
Sure. I copied the commands from https://github.com/ipfs/interop - can you PR into the README there to update them? |
I guess something I don't like about this is that a PR could pass CI and be merged, then break master due to an interop test failing. Maybe we should run them on every branch? |
We should probably include them as a devDep at that point, otherwise specifying which version of the interop tests should run becomes hard (e.g. breaking changes, new tests, etc). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty cool
3fbf6e8
to
1d1fd99
Compare
@@ -199,7 +199,7 @@ | |||
"hat": "0.0.3", | |||
"interface-ipfs-core": "^0.113.0", | |||
"interop-ipfs": "ipfs/interop#add-bin", | |||
"ipfsd-ctl": "~0.46.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scary. What happened?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm. Read the commit message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a nutshell we have too many modules that are written as if they exist independently of each other.
- A patch-release of
multiformats/mafmt
pulled inmultiaddr@v7
, broke this comparison and caused tests to fail with:
AssertionError: expected '/ip4/127.0.0.1/tcp/45467/ipfs/QmXLcgh85JSbd2v6zsFgu68fjeqsaqGLapFD5MxHLpEwtW/ipfs/QmXLcgh85JSbd2v6zsFgu68fjeqsaqGLapFD5MxHLpEwtW\n' to deeply equal '/ip4/127.0.0.1/tcp/45467/ipfs/QmXLcgh85JSbd2v6zsFgu68fjeqsaqGLapFD5MxHLpEwtW\n'
+ expected - actual
- Only forward, so I upgraded
js-ipfs
andjs-ipfs-http-client
tomultiaddr@v7
, tests passed so publishedipfs-http-client@35
with the newmultiaddr
with a view to PRing it intojs-ipfs
ipfsd-ctl
upgradedipfs-http-client
to35
in0.46.1
- Lots of the
js-ipfs
dependency tree hasmultiaddr@v6
, cannot upgrade right now because of the scope of API changes - I asked @vasco-santos to downgrade
multiaddr
inmafmt
instead, published asv0.6.10
- Now we need
[email protected]
which is in[email protected]
but not[email protected]
so as a stop gap, pin[email protected]
, so we can run the interop tests on CI and fix the build
This weird js-ipfs
, js-ipfs-http-client
, js-ipfsd-ctl
version dance is what I'm talking about in #2446
Anyway, next steps:
- Assuming CI passes on this PR, integrate interop test running with
v0.38.x
branch (today) - Run tests of webui, desktop, companion, etc as per new release process (tomorrow)
- Start community dev testing of
v0.38.x
(tomorrow/Monday) - Upgrade libp2p, ipfs-repo, etc (Monday/Tuesday/Most of next week)
e729ce6
to
a6a1b0b
Compare
a6a1b0b
to
ceb15c8
Compare
Also tag last successful build and ensure we do not get stealth multiaddr@v7
ceb15c8
to
0ebf21b
Compare
I'm going to merge this because the new interop bits of the build are passing, the failing bits are the electron-render related stuff @hugomrdias is fixing in ipfs-inactive/js-ipfs-http-client#1105 |
This will need updating to only run on master & add
npx aegir update-last-successful-build
from ipfs/aegir#419 as a post-build step