gotwarlost / istanbul

Yet another JS code coverage tool that computes statement, line, function and branch coverage with module loader hooks to transparently add coverage when running tests. Supports all JS coverage use cases including unit tests, server side functional tests and browser tests. Built for scale.
Other
8.7k stars 786 forks source link

Support for defaults when destructuring parameters #613

Open NotTheUsual opened 8 years ago

NotTheUsual commented 8 years ago

Hey, Node 6 just shipped with a whole bunch of extra ES6 features, including defaults for destructured parameters, e.g.

function destructure({test = 'not set'}) {
  return `value of "test" is: ${test}`;
}

Mocha tests understand this without problem, but running the same tests through Istanbul (1.0.0-alpha.2) fails: the default is ignored, and any unset values come through as undefined. I appreciate that this may be beyond your control (i.e. the responsibility of a dependency), but figured it was worth flagging.


The failing test, for reference:

it('can use a default value for the test', () => {
  expect(index.destructure({})).to.equal('value of "test" is: not set');
});

The Mocha run returns value of "test" is: not set, the Istanbul run returns value of "test" is: undefined

tomyf commented 8 years ago

It also fails for Istanbul 0.4.3 as well. As far as I tried, it is working for default values when not destructuring, but it is not working as @NotTheUsual said for destructuring with default values.

holm commented 8 years ago

This would be great to have fixed. We were looking to get rid of Babel when upgrading to Node 6, but since Istanbul causes default values to not work with destructuring, we have to keep Babel in there.

tswaters commented 8 years ago

This looks like a bug in escodegen... the AST includes the destructured parameter when it hits that point, and when it comes out, the assignments in destructured objects are lost. I've created an issue in their queue for this: https://github.com/estools/escodegen/issues/296

gotwarlost commented 8 years ago

istanbul@next uses babel under the covers for instrumentation. That one should work for this use case. Can you try using this version?

tomyf commented 8 years ago

Testcase :

const tmp = ({test = 'not set'} = {}) => test
// exports...
const tmp = require('../src/tmp'); // function defined above
const {assert} = require('chai');

describe('tmp', () => {
    it('should return the correct value', () => {
        assert.strictEqual(tmp({test: 'AA'}), 'AA');
    });
    it('should return the default value when test is not defined', () => {
        assert.strictEqual(tmp({}), 'not set');
    });
    it('should return the default value when no arguments are given', () => {
        assert.strictEqual(tmp(), 'not set');
    });
});

:+1:

DavidGDD commented 8 years ago

@tswaters has opened a pull-request (https://github.com/estools/escodegen/pull/297) to solve this... it seems a problem from escodegen.

We will expect that Istanbul update the dependency as soon as posible escodegen has released the merge.

Until then I'm using isparta with babel and es2015 preset, but i'm trying to avoid babel (and all its dependencies). some one knows an alternative?

tswaters commented 8 years ago

This may not be ideal, but you could always manually patch the escodegen inside your node_modules with the changes from my PR. It should be noted the one in npm doesn't have this commit so you'll need to apply that as well. Seems like istanbul@next fixes this though so it might be easiest to switch your dep to that.

DavidGDD commented 8 years ago

Escodegen has generated a new version (1.8.1) which corrects this issue!

webliving commented 7 years ago

@tomyf i used istanbul@1.1.0-alpha.1 too, why not ? 😄