zeppelinos / zos-cli

:warning: Deprecated repo in favour of https://github.com/zeppelinos/zos
https://zeppelinos.org/
101 stars 25 forks source link

Add tests for commands parsing and option handling #346

Closed bakaoh closed 6 years ago

bakaoh commented 6 years ago

Fix: #298

spalladino commented 6 years ago

Awesome work @bakaoh, thanks a lot! I'd refactor the code to reduce repetition and make more usage of before/after blocks, thus making the tests themselves as slim as possible.

So, instead of doing the following:

it('should call add script with an alias and a filename', function(done) {
  var addFake = sinon.fake(function () {
    expect(addFake.calledOnce).to.equal(true)
    expect(addFake.calledWithExactly({ contractsData: [ { name: 'ImplV1', alias: 'Impl' } ] })).to.equal(true)
    addStub.restore()
    done()
  })
  var addStub = sinon.stub(add, 'default').callsFake(addFake)
  program.parse(['node', 'zos', 'add', 'ImplV1:Impl', '--skip-compile'])
});

I'd try to build something like this, where the before and after blocks can be reused by other tests:

beforeEach('set up stubs', function () {
  this.addFake = sinon.fake();
  this.addStub = sinon.stub(add, 'default').callsFake(addFake);
});

afterEach('restore', function () {
  this.addStub.restore();
});

it('should call add script with an alias and a filename', function(done) {
  program.parse(['node', 'zos', 'add', 'ImplV1:Impl', '--skip-compile'])
  this.addStub.should.have.been.calledOnceWithExactly({ contractsData: [ { name: 'ImplV1', alias: 'Impl' } ] })
});

(I'm using sinon-chai on the example above, to have more declarative assertions)

Also, and note that I'm not that familiar with sinon, maybe we could use the sinon.replace syntax, instead of needing to create a fake and a stub:

beforeEach('set up stubs', function () {
  sinon.replace(add, 'default', sinon.fake())
});

afterEach('restore', function () {
  add.default.restore()
});

it('should call add script with an alias and a filename', function(done) {
  program.parse(['node', 'zos', 'add', 'ImplV1:Impl', '--skip-compile'])
  add.default.should.have.been.calledOnceWithExactly({ contractsData: [ { name: 'ImplV1', alias: 'Impl' } ] })
});

What do you think? We could have a single beforeEach block that stubs out all the scripts we need for that test suite, plus runWithTruffle, and then each test should just be a two-liner with the arguments to the command, and the arguments to the stub. We could also make these blocks reusable across all command tests, but let's start with add for now.

bakaoh commented 6 years ago

@spalladino -- actually i used beforeEach/afterEach and sinon.replace at first just like your snippet. But i can not find a way to wait for program.parse to finish execute command. Please let me know if you have any idea

spalladino commented 6 years ago

I'd check if commander has any event to notify when the execution of an action has completed, and try to use that.

If not possible, then I'd try to abstract everything into a function that takes care of the boilerplate, so each test is as slim as possible. For example, something like:

function itShouldParse(name, cmd, args, cb) {
  it(name, function (done) {
    let stub, fake;
    fake = sinon.fake(function () {
      cb(fake);
      stub.restore();
      done();
    })
    stub = sinon.stub(cmd, 'default').callsFake(fake)
    program.parse(args)
  });
}

itShouldParse('add with an alias and filename', add, ['node', 'zos', 'add', 'ImplV1:Impl', '--skip-compile'], function (fake) {
  fake.should.have.been.calledOnceWithExactly({ contractsData: [ { name: 'ImplV1', alias: 'Impl' } ])
});

I used the stub/fake syntax you presented originally, but it may work with replace as well. WDYT?

bakaoh commented 6 years ago

your itShouldParse make each test so slim, but it's not flexible enough for tests that need more than 1 stub so i modified it a bit

bakaoh commented 6 years ago

It's done. I have changed few things in push and init commands since i guess they're bugs, and made a change in session command to make it testable. Please preview them

spalladino commented 6 years ago

Merged! Thanks so much for your efforts, @bakaoh!