Interoperability testing for IPFS

daviddias commented 6 years ago

Taking a page from https://github.com/libp2p/interop/issues/1, let's start working on the interop tests for IPFS. Fortunately, in IPFS land we do have a concept of the daemon and we already have a batch of tests that we can import to this repo https://github.com/ipfs/js-ipfs/tree/master/test/interop making things so much simpler! 🎉

dryajov commented 6 years ago

What are we missing in terms of interop test? Let's make a list.

victorb commented 6 years ago

Had a call with @dryajov and to make sure we're all on the same page (cc @diasdavid), here is my thoughts about what ipfs/interop is and what we need to do to make sure ipfs/interop becomes easy to maintain and provides a lot of value.

First, what ipfs/interop is. Shortly, ipfs/interop tests that different implementations can work with each other. Adding a file on a js-ipfs node should be gettable on a go-ipfs node, for example.

Current issues with our ipfs/interop tests:

They contain accidental complexity, making them hard to write and maintain
Test cases are not isolated. This is also because of how the tests are run
Tests run slow, sometimes hitting timeouts
Random failures. Unclear if because of tests itself or system being tested
Some tests test more than they should

These problems are not only for ipfs/interop but in general with our tests. ipfs/interop can be a good place to start with "doing testing right" (TM).

Accidental complexity refers to complexity that is not there because it has to (some things are just complex) but rather because lack of time to do something proper (also called a hack) or just a lack of knowledge on how to structure it.

First thing to have in mind when creating or modifying existing tests is that we should treat them as production code, meaning that the code is important, should be well abstracted and have the same importance as the source code itself.

Reducing complexity in the test cases, reusing functionality when we can, refactor code as we move along and cache calls are just a few things we can do.

I think we can solve this by placing focus on having a internal testing DSL, making the test-cases as small as possible, easy to read and maintain.

Let's make a example from a existing test case, a very simple one. It simply sets up two nodes with pubsub, sends one message from one of them and makes sure it arrives at the other node. It currently looks like this:

  describe('ascii data', () => {
    const data = Buffer.from('hello world')

    it('publish from Go, subscribe on Go', (done) => {
      const topic = 'pubsub-go-go'
      let n = 0

      function checkMessage (msg) {
        ++n
        expect(msg.data.toString()).to.equal(data.toString())
        expect(msg).to.have.property('seqno')
        expect(Buffer.isBuffer(msg.seqno)).to.be.eql(true)
        expect(msg).to.have.property('topicIDs').eql([topic])
        expect(msg).to.have.property('from', goId)
      }

      series([
        (cb) => goD.api.pubsub.subscribe(topic, checkMessage, cb),
        (cb) => goD.api.pubsub.publish(topic, data, cb),
        (cb) => waitFor(() => n === 1, cb)
      ], done)
    })
  })

A huge piece of code for something so simple. I want us to be able to do this instead:

describe('pubsub', () => {
  it('can publish and subscribe from same node', () => {
    const node = CreateNode()
    SendMessage(node, 'hello world')
    AssertReceivedMessages(1, 'hello world')
  })
})

(since writing this comment, I've made a full example of how it could work [highlighted is the test case itself, the other things are just the testing setup])

Which language implementation we're using, should be hidden and automatically repeated. Instead of manually defining the combinations, they should be generated.

Test run would output something like:

OK - go - pubsub - can publish and subscribe
OK - js - pubsub - can publish and subscribe

Adding support for multiple nodes should be a CreateNodes(2) call, and test suite can infer all combinations.

OK - go <> go - pubsub - can publish and subscribe
OK - js <> js - pubsub - can publish and subscribe
OK - go <> js - pubsub - can publish and subscribe
OK - js <> go - pubsub - can publish and subscribe

Point being, most code in after/before/afterEach/beforeEach should be isolated into the test cases, test cases should not depend on order and only the neccessary logic should be in the test case itself. Rest should be internal.

Regarding isolation. Currently a lot of tests runs on the machines directly, as we want to test platform differences. For ipfs/interop, we have two options, one which is more helpful than the other.

The first and simple measure is to run tests in containers. Then we can run multiple tests on one worker in CI. But, we remove the ability to cross-platform test, which we might not want to give up for ipfs/interop.

Second and what I'm looking into right now, is Software Defined Networks (SDN). This gives us a way of modeling network topologies via software. We can make a much wider selection of tests with weird network topologies, and we'll have use for it in more places than just ipfs/interop. This also gives us isolation, and we can still run tests on multiple platforms.

Isolating the tests better will also help with timeouts that happens because there is some daemon that lingers and gets connected to the next test run somehow. It also gives us the relaxation regarding ports, as each node will have it's own NIC.

Regarding random failures, we have to first figure out if it's because of the tests, or if the system we're testing, is actually not as stable as we think. Race-conditions and other things will appear on different systems and could be the source of the problem. I think if we isolate tests properly, we can dive deeper into figuring this out.

Lastly, tests testing more than they should. We're mixing types of tests all over the place (ipfs/js-ipfs includes a go-ipfs daemon in tests sometimes).

We need a clear separation between unit, functional, interop and protocol tests, and they should not test the same things. We need to avoid testing the same thing all over again, but in a (unrelated) different scenario as much as we can.

I've began experimenting with a testing DSL with the purpose of making the tests as clear and easy understandable as possible. I need to complete it and make it do more, to make sure it doesn't get too complex for what we want to do.

My suggested action plan:

[ ] Implement PoC of internal testing DSL (demo above) for ipfs/interop
[ ] Have @dryajov and @VictorBjelkholm together implement circuit tests with internal testing DSL
[ ] Have @VictorBjelkholm add some sort of SDN for ipfs/interop tests
- Might be able to just use docker networks for this, but then we can't run cross-platform :/

alanshaw commented 6 years ago

Massive +1 on test isolation!

daviddias commented 6 years ago

@VictorBjelkholm this reads to me as a rehashing or continuation of the conversation we had in Lisbon early January. Just to make sure we are still in the same page, I'm all in for improved tests, fast CI and reduced complexity \o/

It is good to have these written down as notes, thank you. However, I believe the motivation behind this discussion with @dryajov was https://github.com/ipfs/interop/issues/12 and https://github.com/ipfs/interop/issues/11 which could benefit from having the tests structured in this new way, but it shouldn't be a blocker for circuit-testing.

Let's make a example from a existing test case, a very simple one. It simply sets up two nodes with pubsub, ....

Nothing is stopping you from doing this today, in the end it is up to the test writer. I wouldn't call that example a DSL for testing though, it is just putting a bunch of assertions behind a function call. I can find you tests that are written that way.

Second and what I'm looking into right now, is Software Defined Networks (SDN). This gives us a way of modeling network topologies via software.

That's the job for InterPlanetary Test Lab

Lastly, tests testing more than they should. We're mixing types of tests all over the place (ipfs/js-ipfs includes a go-ipfs daemon in tests sometimes).

Not true anymore since we moved the interop tests to this repo.

Massive +1 on test isolation!

Agreed, as discussed on https://github.com/ipfs/interface-ipfs-core/issues/189

There are other things, for example a huge regression was introduced with the new version of ipfsd-ctl -- https://github.com/ipfs/js-ipfsd-ctl/pull/176 -- which removed from every single test the ability to pick the keysize -- https://github.com/ipfs/js-ipfsd-ctl/issues/188#issuecomment-365893965 --, making tests ridiculously slow. This needs to be fixed asap.

dryajov commented 6 years ago

It is good to have these written down as notes, thank you. However, I believe the motivation behind this discussion with @dryajov was #12 and #11 which could benefit from having the tests structured in this new way, but it shouldn't be a blocker for circuit-testing.

I agree, this is not a blocker to the current circuit testing, but potential ways to improve them in the future. My intent with #12 was to document my general experience and gather ideas on how to improve them in the future - I'm glad that we have a conversation started around it now. 👍

ipfs / interop

Interoperability testing for IPFS #2