Skip to content
This repository was archived by the owner on Aug 1, 2023. It is now read-only.

chore: update ipfsd-ctl version and use daemon.js util file#86

Merged
achingbrain merged 16 commits into
masterfrom
chore/update-ipfsd-ctl
Oct 21, 2019
Merged

chore: update ipfsd-ctl version and use daemon.js util file#86
achingbrain merged 16 commits into
masterfrom
chore/update-ipfsd-ctl

Conversation

@PedroMiguelSS

@PedroMiguelSS PedroMiguelSS commented Sep 23, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Completes phase 1 and 2 to tackle #84 as referenced here.

Some tests were skipped due to an inconsistency: the same content is getting different hashes when added to a go daemon and a js daemon. The skipped tests are:

files.js file

  • trickle DAGs

trickle

  • rabin chunker

rabin

  • rabin chunker small chunks

rabin small

⚠️These tests must be unskipped as soon as the problem gets solved.

@PedroMiguelSS PedroMiguelSS force-pushed the chore/update-ipfsd-ctl branch 3 times, most recently from f5c267f to da93ced Compare September 30, 2019 14:31
@PedroMiguelSS PedroMiguelSS changed the title WIP: update ipfsd-ctl version and use daemon.js util file update ipfsd-ctl version and use daemon.js util file Sep 30, 2019
@PedroMiguelSS PedroMiguelSS changed the title update ipfsd-ctl version and use daemon.js util file chore: update ipfsd-ctl version and use daemon.js util file Sep 30, 2019
@PedroMiguelSS PedroMiguelSS mentioned this pull request Sep 30, 2019
Closed
vasco-santos
vasco-santos suggested changes Oct 1, 2019
View reviewed changes
Comment thread test/cid-version-agnostic.js Outdated
Comment thread test/exchange-files.js
Comment thread test/files.js Outdated
Comment thread test/ipns.js Outdated
Comment thread test/pin.js
Comment thread test/repo.js Outdated
Comment thread test/repo.js Outdated
@achingbrain

achingbrain commented Oct 1, 2019

Copy link
Copy Markdown
Member

I'm a little concerned by the skipped tests around different hashes.

These tests are run on the current release & master of js-IPFS and all pass - why has converting them to async/await made them fail? Or is it something to do with a new dep?

@PedroMiguelSS PedroMiguelSS force-pushed the chore/update-ipfsd-ctl branch from da93ced to c068606 Compare October 2, 2019 10:44
@PedroMiguelSS

PedroMiguelSS commented Oct 2, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I've debugged a little bit more and I found out that the problem on both remove a child shared by multiple pins and print same pins tests (pin.js file) was related to the options that were being passed to the spawn() method. The default config had sharding option enabled and this was causing the problem on the latest versions of ipfsd-ctl (v0.47.1). However, it's fixed and these 2 tests are passing now 🎉

However, I still have 3 skipped tests on files.js file as you can see in the description of this PR. I found out that this problem only occurs on versions of ipfsd-ctl greater than v0.46.0. Why? There is one commit that updates several dependencies. One of the updated dependencies is the ipfs-http-client and it was updated from v34.0.0 to v35.1.0. Thus, there was a breaking change and the add method was changed as we can see here.

Can I get some help on this?
@achingbrain @alanshaw @hugomrdias

hugomrdias
hugomrdias suggested changes Oct 4, 2019
View reviewed changes
Comment thread test/files.js Outdated
Comment thread test/utils/timeout.js Outdated
vasco-santos
vasco-santos reviewed Oct 4, 2019
View reviewed changes
Comment thread test/utils/daemon.js Outdated
vasco-santos
vasco-santos approved these changes Oct 7, 2019
View reviewed changes

@vasco-santos vasco-santos left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me!

Now we need to decide on how to iterate with the skipped tests. Taking into attention @PedroMiguelSS analysis, it looks like a problem not related with this. Also, the same configurations are provided. It would be great to not skip the tests, but if anyone is now available to help debugging it, we should probably create issues to get to them as soon as possible, and not block the refactor, as we are upgrading a lot of dependencies here, some of them several breaking releases. @achingbrain what is your feeling here?

hugomrdias
hugomrdias approved these changes Oct 7, 2019
View reviewed changes

@hugomrdias hugomrdias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets merge this, @PedroMiguelSS can you please create an issue referencing this ?

@hugomrdias

hugomrdias commented Oct 7, 2019

Copy link
Copy Markdown
Member

@PedroMiguelSS check whats happening with the CI

@PedroMiguelSS

PedroMiguelSS commented Oct 7, 2019

Copy link
Copy Markdown
Contributor Author

@hugomrdias, issue: #88

@PedroMiguelSS PedroMiguelSS force-pushed the chore/update-ipfsd-ctl branch from 62f867a to facc384 Compare October 9, 2019 13:28
@PedroMiguelSS PedroMiguelSS force-pushed the chore/update-ipfsd-ctl branch from facc384 to 4f38311 Compare October 9, 2019 13:29
@hugomrdias hugomrdias mentioned this pull request Oct 14, 2019
Merged
@hugomrdias

hugomrdias commented Oct 14, 2019

Copy link
Copy Markdown
Member

@achingbrain we still have a couple of timeout error coming from the go daemon on windows by this should be good to go, can you review it again pls

achingbrain
achingbrain reviewed Oct 15, 2019
View reviewed changes
Comment thread test/cid-version-agnostic.js Outdated
achingbrain
achingbrain reviewed Oct 15, 2019
View reviewed changes
Comment thread test/circuit/browser.js
achingbrain
achingbrain reviewed Oct 15, 2019
View reviewed changes
Comment thread test/files.js Outdated
// Somehow, the hashes for the same content are not equal
// This problem was also detected on
// both 'remove a child shared by multiple pins' test and 'print same pins'
it.skip('trickle DAGs', () => {

@achingbrain achingbrain Oct 15, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this (and the other two) still need to be skipped?

@PedroMiguelSS PedroMiguelSS Oct 16, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 3 tests cannot be unskipped as they are still failing. Issue #88 has further details about this problem.
Should we block this PR until we get this solved?

@achingbrain achingbrain Oct 16, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm taking a look at those failing tests

@achingbrain achingbrain Oct 16, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem was that both invocations of testHashesAreEqual in each test was consuming data from the same ReadableStream - something in the timing of the execution of the tests may have changed which meant that one invocation was missing the first few data events.

Refactoring the tests so each invocation of testHashesAreEqual gets it's own data stream seems to have solved the problem.

@achingbrain achingbrain Oct 16, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though having said that I do seem to be getting different CIDs from js-IPFS master on the cli if the daemon is on or off when --trickle is specified. 😬

@achingbrain achingbrain Oct 16, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ipfs-http-client wasn't inspecting the right field for the --trickle arg - the fix is in ipfs-inactive/js-ipfs-http-client#1129 and I'll backport it to ipfs-http-client@38.0.x when it's merged.

@achingbrain achingbrain Oct 16, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Urgh, no, it's more complicated that that, sorry - tired. ipfs-http-client is doing the right thing - it takes a trickle key as part of it's options and sends that over http. The cli in ipfs takes a trickle argument but turns it into strategy: 'trickle' (as that's what's required by the unixfs-importer) and passes that to ipfs.add* and the http api endpoint does the same thing.

The right thing to do is for ipfs-http-client, the cli and the http api endpoint to all pass trickle: true to ipfs.add (as per the interface spec) and have that do the conversion to strategy: 'trickle'.

  • = actually ipfs._addAsyncIterator but ipfs.add just passes it's options straight through so same same.

@achingbrain achingbrain Oct 16, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ipfs/js-ipfs#2541

achingbrain
achingbrain reviewed Oct 15, 2019
View reviewed changes
Comment thread test/ipns-pubsub.js Outdated
achingbrain
achingbrain reviewed Oct 15, 2019
View reviewed changes
Comment thread test/ipns-pubsub.js Outdated
achingbrain
achingbrain reviewed Oct 15, 2019
View reviewed changes
Comment thread test/pin.js Outdated
@achingbrain

achingbrain commented Oct 15, 2019

Copy link
Copy Markdown
Member

Tiny nits, just needs the added .skips removing really.

Pedro Santos and others added 2 commits October 16, 2019 08:29
chore: code review changes
fca8fa0
@achingbrain
fix: give each invocation it's own data stream
cf57d6b 
With all the added promises I think the order of execution had changed
so one invocation would not recieve all the data events.
@achingbrain

achingbrain commented Oct 17, 2019

Copy link
Copy Markdown
Member

I think this is good to go, will merge when CI passes. Looks like some flaky tests in there.

@achingbrain

achingbrain commented Oct 17, 2019

Copy link
Copy Markdown
Member

Famous last words, perhaps.

@achingbrain

achingbrain commented Oct 18, 2019

Copy link
Copy Markdown
Member

I've replicated the failing tests locally, seems this is the fix: ipfs-inactive/js-ipfs-http-client#1130

@achingbrain

achingbrain commented Oct 21, 2019

Copy link
Copy Markdown
Member

Tests passing, hooray!

@achingbrain achingbrain merged commit b7aa313 into master Oct 21, 2019
@momack2

momack2 commented Oct 22, 2019

Copy link
Copy Markdown

CONGRATULATIONS ON THE AWESOME MERGE, @PedroMiguelSS and @achingbrain!!

@achingbrain achingbrain mentioned this pull request Oct 23, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@achingbrain achingbrain achingbrain approved these changes

@hugomrdias hugomrdias hugomrdias approved these changes

@vasco-santos vasco-santos vasco-santos approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@PedroMiguelSS @achingbrain @hugomrdias @momack2 @vasco-santos