Faster tests

[victorb]

Pretty basic changes, in short:

• Make the daemon cli tests faster by not relying on timeouts and streaming stdout (removed pull-streams here as well) Made the daemon tests go from 6 minutes to 30 seconds
• Inline all the requires in the cli part of js-ipfs. Makes the cli (without arguments) go from 0.9 seconds to 0.3 seconds, speeding up the CLI tests from 12 minutes to 7 minutes
• Modify the load tests for pubsub in interface-core-ipfs to be more like mini-load tests

More to come

Depends on ipfs-inactive/interface-js-ipfs-core#263

[victorb]

Preliminary testing shows that CI runs on Jenkins goes from ~27 minutes to about ~15 minutes with these changes.

Another great side-effect is that the CLI is generally much faster with and without args! Might no see a difference when adding large files tough.

[daviddias]

Please follow https://github.com/ipfs/community/blob/master/js-code-guidelines.md#commits

[daviddias]

LGTM. Just needs: rebase master to fix merge conflicts, update deps and fix commit messages.

[travisperson]

Non-blocking discussion:

I haven't really been keeping up with all bundlers, but is the use of require outside of the global scope of a module handled correctly for most bundlers? (webpack, rollup, browserify).

All these changes are to the CLI which won't (shouldn't) be bundled, so no issue here. However, because we are now introducing this pattern we will need to make sure it doesn't leak into other areas. The main issue is that require is a blocking call, which will result in a pause on the first use of a command while the modules are loaded and cached.

[daviddias]

@travisperson you are absolutely correct in both the potential issue and how the current change doesn't necessarily cause it. Let's make sure to do extra diligence on PR review so that this antipattern doesn't propagate.

@victorbjelkholm, this is a new error:

  1) cli daemon do not crash if Addresses.Swarm is empty:

     Error: Daemon did not get ready in time

      at Timeout.setTimeout [as _onTimeout] (test/cli/daemon.js:81:14)

[alanshaw]

Hey, just something for the future, it would be helpful if these changes were sent as separate PRs 😜

The speed ups you've managed to achieve are super impressive and I'm all 👍 for that.

That said, the inline requires thing gives me the creeps. It's just a bit unusual to see all the requires inlined like that. I would normally expect them to be at the top of the file. It's also touched a lot of files and we'll have to remember to keep inlining require calls forever or the benefit will drop off as time progresses.

I think that moving this idea up the module tree a little would yield similar speed 'em ups. Instead of inlining every require, why not just load modules relevant to the command being executed? It would probably not touch quite so many files and leave them in a bit more of a familiar state for future and current contributors. We'd also not have to remember to keep inlining requires, or that it should only be done in the CLI and not the rest of the code base.

What do you think?

[vasco-santos]

I would also expect the requires at the top of the file. With this approach, it is easier to understand which modules are being used in that scope, which I believe to be essential in a big project like this.

Therefore, I would go for the solution proposed by @alanshaw

[victorb]

Hey, just something for the future, it would be helpful if these changes were sent as separate PRs stuck_out_tongue_winking_eye

There is basically just two changes here. One is fixing a daemon test and secondly is making the cli start faster, which leads to faster tests. But you're right, could have split them up. Main reason they are together is that I wanted to compare the duration of current master jobs and this branch duration, to make sure it's actually speeding it up.

The speed ups you've managed to achieve are super impressive and I'm all +1 for that.

Thanks :)

That said, the inline requires thing gives me the creeps. It's just a bit unusual to see all the requires inlined like that. I would normally expect them to be at the top of the file. It's also touched a lot of files and we'll have to remember to keep inlining require calls forever or the benefit will drop off as time progresses.

I agree, it's not a perfect solution but rather the only one I found. Would be happy to hear alternative solutions if it actually provides the same benefits. We don't have to inline all the require calls, just the ones in the top-level cmds.

I think that moving this idea up the module tree a little would yield similar speed 'em ups. Instead of inlining every require, why not just load modules relevant to the command being executed? It would probably not touch quite so many files and leave them in a bit more of a familiar state for future and current contributors. We'd also not have to remember to keep inlining requires, or that it should only be done in the CLI and not the rest of the code base.

I'm not sure how this would work? I inlined them at the top-most part I could find. If they are not inlined in the commands, where would they be inlined? "why not just load modules relevant to the command being executed" is exactly what happens here, but maybe I don't understand you correctly.

I would also expect the requires at the top of the file. With this approach, it is easier to understand which modules are being used in that scope, which I believe to be essential in a big project like this.

If we can find a way to make that work + lazily load modules, I would prefer that as well. But I could not find a way to make that work. I would love to see a practical example of any other solution.

[victorb]

Re new error "this is a new error"

Not sure what's going on here. The test passes when run in isolation, but fails when run as a part of the full suite. It passes locally (linux) for me, both isolated and part of the full suite. Looking into it.

[victorb]

I'm pretty sure I found a fix. Embarrassingly, I had clearInterval instead of clearTimeout 🤦‍♂️

[victorb]

We have some race-conditions or something going on from interface-ipfs-core (dag + files suites). I can't reproduce this on either linux or macOS locally.

old

[victorb]

Seems to finally pass all the tests, @diasdavid can you review this again?

[daviddias]

With #1350, shall we close this one, @victorbjelkholm ?

[victorb]

@diasdavid yup! Thanks @alanshaw \o/