feat: modular interface tests

[alanshaw]

This PR updates js-ipfs-api to use the new interface-ipfs-core tests. This gives us fine grained control over what tests we run.

Please see the description on the PR for more information on the reasons behind this change.

This also enables a bunch of tests that weren't previously being run 🎉 ...and removes a LOT of duplicated code for running the interface tests.

requires ipfs-inactive/interface-js-ipfs-core#290

[daviddias]

Hm... is this really the preferred approach? I foresee people changing the test title and not understanding why things are failing. I kind of liked when the tests explicitly communicated that it was known it wasn't working with go-ipfs or js-ipfs.

[alanshaw]

You're absolutely right, there's a trade off here that I should have drawn more attention to - if a test name is changed in interface-ipfs-core and it is skipped in the consumer then it will begin to fail.

These PRs propose to put the responsibility on the implementation to know what tests should be skipped. I don't believe the interface should change based on the implementation/platform/environment. I also don't believe the interface should need to know about all possible implementations. It feels appropriate to me that the implementation should know and encapsulate this information, leaving interface-ipfs-core as a point of truth, dictating the interface that implementations need to conform to.

I see the advantages as:

• Not having to send a separate PR to interface-ipfs-core when enabling already skipped tests or temporarily disabling flaky/buggy/not implemented tests - these PRs are not really for interface-ipfs-core, they are for js-ipfs or js-ipfs-api, but they leak into interface-ipfs-core because that's currently the only way to skip/unskip
• Having a central location when you can easily see and manage all the tests that are skipped
• Not having to maintain hacks and skips for tests based on all implementations that will ever exist in interface-ipfs-core e.g. withPhp, withJava, withRuby
  • Also if another Go implementation or JS implementation pops up, I don't want the interface to be skipping tests for them when it might not be appropriate to do so
• Discouraging misuse of implementation specific checks, for example I've seen !withGo be used when devs actually mean withJs, which could easily cause problems in the future when being used against a Java or a Ruby backend (super minor, but worth noting)

The disadvantage:

• If a test name is changed in interface-ipfs-core and it is skipped in the consumer then it will begin to fail

I rate the disadvantage as low impact. The frequency of test name changes is low (IMHO) and the chances of a test name changing and it also being a test that is skipped is also low. We should also be striving for no skips which would eradicate the risk completely.

It's your call, and I'm happy to move these back into interface-ipfs-core - I believe the other changes in these PRs are still valuable.

[alanshaw]

Also worth mentioning - we already have a scattering of where this information is held. We have implementation specific skips in interface-ipfs-core (go/js), but platform dependent (browser/node.js) skips in js-ipfs.

For example, some suites don't work in the browser so aren't included https://github.com/ipfs/js-ipfs/blob/2e9418465c863b129300ea5f0c8b32590edc7729/test/core/interface/interface.spec.js#L17-L23 and we have the same thing for functionality that isn't implemented e.g. repo tests simply just not included in js-ipfs because it's not all implemented yet.

These PRs would put all of that information in the same place, which I think is a win.

I'd like to at least try things this way around and if it becomes an issue then we change it back?

What do you think @diasdavid - is worth a try? I'd like to move forwards on these PRs soon because they're becoming difficult to keep up to date with master!

Thank you, much ❤️

[daviddias]

Thank you so much for putting so much care and thought into this, @alanshaw ❤️

I won't block you from moving forward but I have one last request. Can we treat the test suite as something definitive and everytime there is something that is skipped (i.e. does not work with go-ipfs) it is up to the implementation to skip the test but also print a log saying that says "go-ipfs incompatible" or something that clearly communicates to the dev that there is missing stuff?

[alanshaw]

I'll have a think about what to do. Some ideas off the top of my head:

Option 1

// instead of
{
  skip: ['test1', 'test2']
}
// if passed as an object then interpret it as { skippedTest: 'reason' }
{
  skip: {
    'test1': 'go-ipfs incompatible',
    'test2': 'another reason'
  }
}

and causes output in the console like:

...
  ✓ other test
  - test1 (go-ipfs incompatible)
  - test2 (another reason)
...

---

Option 2

// instead of
{
  skip: ['test1', 'test2']
}
// if skip array item is an object, extract name & reason
{
  skip: [
    { name: 'test1', reason: 'go-ipfs incompatible' },
    { name: 'test2', reason: 'another reason' }
  ]
}

Too verbose?

---

Option 3

// instead of
{
  skip: ['test1', 'test2']
}
// explicit skip reasons object
{
  skip: ['test1', 'test2'],
  skipReasons: {
    'test1': 'go-ipfs incompatible',
    'test2': 'another reason'
  }
}

Too weird i think, and detached from skip test names.

...I think option 1 is my fav

[daviddias]

I would say Option 1 given the example but since some test titles are actually long, Option 2 might serve us better.

[alanshaw]

@diasdavid is it nice?:

skip specific test

skip entire subsystem

[daviddias]

Why not use 1024 for all tests? It would make things way faster.

[daviddias]

Seems this was actually lost with this change, before all tests used 1024.

[alanshaw]

They still are, the defaultCommonFactory passed to other suites uses 1024 by default https://github.com/ipfs/js-ipfs-api/pull/785/files/519bbf5c4374317d5355d9bddfb2fc5862ddd16d#diff-b4ff00e5aeab8b441e96f3d0b1082a2eR12

[daviddias]

Understood! Thanks :)

[daviddias]

Why skip?

[alanshaw]

These were not being run before and they fail right now - I'll add a comment

[daviddias]

Why skip?

[alanshaw]

These were not being run before and they fail right now - I'll add a comment

[alanshaw]

Note to self: this needs to be moved back into dependencies before merging as it's used in the main code

[daviddias]

@alanshaw does it fail when run?

[alanshaw]

Yes unfortunately. It was just skipped in interface-ipfs-core before with no explanation.

  1) interface-ipfs-core tests
       .bitswap.unwant
         should remove a key from the wantlist:

      Uncaught AssertionError: expected false to equal true
      + expected - actual

      -false
      +true
      
      at ipfsB.bitswap.wantlist (/Users/alan/Code/protocol-labs/interface-ipfs-core/js/src/bitswap/unwant.js:52:61)
      at f (node_modules/once/once.js:25:25)
      at streamToValue (src/utils/stream-to-json-value.js:30:5)
      at concat (src/utils/stream-to-value.js:12:22)
      at ConcatStream.<anonymous> (node_modules/concat-stream/index.js:37:43)
      at finishMaybe (node_modules/readable-stream/lib/_stream_writable.js:630:14)
      at afterWrite (node_modules/readable-stream/lib/_stream_writable.js:492:3)
      at process._tickCallback (internal/process/next_tick.js:63:19)

[daviddias]

y no green CI?

[alanshaw]

@diasdavid I had this all lined up to be merged asap after interface-ipfs-core@0.70 was released.

After 0.70 was released I saw that there were many issues on CI that didn't appear at all when run locally.

Jenkins had a bad day last week coinciding with the release so it's been difficult and slow to debug and resolve the issues. I now have a PR open on interface-ipfs-core to resolve them ipfs-inactive/interface-js-ipfs-core#319 (🤞 hopefully all of them). It would be rad if you have time to take a quick look.

I'm super anxious to get this and ipfs/js-ipfs#1389 merged asap because they're both depending on 0.69.x still. It's confusing and frustrating the community when trying to add tests to interface-ipfs-core right now 😞

[alanshaw]

FYI @diasdavid the 2 DAG tests that are failing here will be fixed by this PR #801

[alanshaw]

@diasdavid gr gr gr greeeeeeen!

[daviddias]

RAD! <3 @alanshaw