Skip to content
This repository was archived by the owner on Mar 10, 2020. It is now read-only.

refactor: tests #519

Merged
merged 2 commits into from
Jan 31, 2017
Merged

refactor: tests #519

merged 2 commits into from
Jan 31, 2017

Conversation

daviddias
Copy link
Contributor

@daviddias daviddias commented Jan 31, 2017 •
edited
Loading

  • remove dedup spawning daemons code
  • put things where they should be
  • make everything use the ipfs-factory

daviddias
daviddias commented Jan 31, 2017
View reviewed changes
test/ipfs-factory/daemon-spawner.js

callback(null, node)
})
], (err) => callback(err, node))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dignifiedquire could you take a look at this function, I'm getting errors setting the config as the config is always empty, not sure what I messed up right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

the setting looks right to me, why do you need the getConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added getConfig to test it, it yield nothing, meaning that the daemon is being created with no config at all

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like init is not being run in ipfsd-ctl for some reason. Did you change something around that usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved, there were too issues, ipfsd out of that and now ipfs for some reason doesn't accept setting Boolean values with the --json flag (??)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The good news is, that we actually never needed 'API.HTTPHeaders.Access-Control-Allow-Credentials': true,

@daviddias daviddias force-pushed the refactor/tests branch 5 times, most recently from a6e586d to 9a1e8e8 Compare January 31, 2017 15:42
@daviddias
Copy link
Contributor Author

daviddias commented Jan 31, 2017 •
edited
Loading

@dignifiedquire @victorbjelkholm could you pull this branch and try to run it and tell me what you see, I'm confused why the heck this is not working, just tested ipfsd-ctl directly and works fine, not sure why here it is going havoc

This is what I see:

  .bootstrap
    1) "before all" hook


  0 passing (749ms)
  1 failing

  1) .bootstrap "before all" hook:
     Uncaught Error: non-zero exit code 1

            while running: /Users/ground-control/code/js-ipfs-api/node_modules/go-ipfs-dep/go-ipfs/ipfs config


            Error: Failed to get config value: " key has no attributes"

      at Stream.listeners.done.once (node_modules/ipfsd-ctl/src/exec.js:39:11)
      at Stream.f (node_modules/once/once.js:25:25)
      at Process.ChildProcess._handle.onexit (internal/child_process.js:215:12)

@daviddias daviddias force-pushed the refactor/tests branch 2 times, most recently from 6dfbae0 to 3137a13 Compare January 31, 2017 15:57
dignifiedquire
dignifiedquire approved these changes Jan 31, 2017
View reviewed changes
Copy link
Contributor

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

Only issue I have is that the file moving completely destroys the git history it seems :(

Other than that, thank you! I was having a real hard time navigating the tests.

@daviddias
Copy link
Contributor Author

Exactly, it was super painful to expand the test suite and also isolate groups of tests, this will give us much more sanity! :) thank you for the review!

@daviddias daviddias merged commit 7b4aafb into master Jan 31, 2017
@daviddias daviddias deleted the refactor/tests branch January 31, 2017 16:56
victorb
victorb reviewed Jan 31, 2017
View reviewed changes
Copy link
Contributor

@victorb victorb left a comment

Choose a reason for hiding this comment

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

In general, looks good

test/bitswap.spec.js
it('.wantlist', () => {
return ipfs.bitswap.wantlist()
.then((res) => {
expect(res).to.have.to.be.eql({
Copy link
Contributor

Choose a reason for hiding this comment

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

could probably be to.be.eql rather than that

test/name.spec.js
})

it('.name.resolve', (done) => {
ipfs.name.resolve(name.Name, (err, res) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, it's no good if the test-cases themselves are not isolated and can't be run by themselves, in this case, name is being set in the publish test.

test/ping.spec.js
const series = require('async/series')
const FactoryClient = require('./ipfs-factory/client')

describe.skip('.ping', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to skip this still?

@victorb
Copy link
Contributor

victorb commented Jan 31, 2017

Reading through this though, makes it clear to me that many of the tests are missing any assertions outside of "does not return an error" which is good for quick, sweeping tests that everything is at least not exploding. But it's not really good in terms of making sure that things actually work. Something we should keep in mind moving forward.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@victorb victorb victorb left review comments

@dignifiedquire dignifiedquire dignifiedquire approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daviddias @victorb @dignifiedquire