Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add live transform of source code before coverage analysis #311

Merged
merged 5 commits into from Feb 26, 2015

Conversation

@ldesplat
Copy link
Contributor

ldesplat commented Feb 21, 2015

This pull request is in reference to Issue #306 .

This allows for anybody to add src level transforms. So in theory, one could write coffee code and be able to use Lab without a compile step beforehand.

Could I get someone to review these changes and let me know if I did anything wrong? I also need some comments on how to get the code coverage for my changes in coverage.js. I really don't understand how you guys are testing it.

I also know, I need to update documentation, but wanted to have some feedback before I invest in that too. Here is how you use it:

  • Specify -T and a module name
    lab -c -T lab-babel ./test

and this is lab-babel.js

var Babel = require('babel-core');

module.exports.extensions = ['.jsx'];
module.exports.transform = function (content) {

    var result = Babel.transform(content, { sourceMap: 'inline', ast: false});
    return result.code;
}

extensions tells lab what other type of files could be loaded in (.coffee, .es6, ...), and transform is the transform method.

@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Feb 21, 2015

If you're lost in coverage, do npm run test-cov-html, it's far more detailed.
As far as the parameter is concerned, I'd rather take a path or a simple name, this would allow you to -T ./lab-babel.js for a local file or -T lab-label for a node module.

EDIT: btw I'm not the maintainer so I have no say in this but I'll help on the review :)

lib/cli.js Outdated

if (Array.isArray(transform.extensions) && typeof transform.transform === 'function') {
options.transform = transform;
} else {

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 21, 2015

Member

Hapi guidelines say new line after }, even before else

@ldesplat ldesplat mentioned this pull request Feb 21, 2015
@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Feb 21, 2015

Shouldn't you test that transformation actually works ? There are only failure tests.

…ption. Change description of needed export module.
@ldesplat

This comment has been minimized.

Copy link
Contributor Author

ldesplat commented Feb 21, 2015

Thanks @Marsup for your comments

Yes, I did not have any comprehensive test coverage because I did not understand how to test it. Gotta wrap your head around testing lab with itself. Anyways, I have added it now and I am only missing 1 line of coverage. I truly don't know how to cover it.

for (var i = 0, il = internals.transforms.length; i < il; ++i) { in coverage.js line 27. Complains about i < il

Changed the API a bit to only transform the files you specify, and it now also transforms when you do not specify coverage option.

var Babel = require('babel-core');

module.exports = [
    {ext: '.jsx', transform: function (content) {

        var result = Babel.transform(content, { sourceMap: 'inline', ast: false});
        return result.code;
    }}
];

Anyways, if people are happy with the implementation and can advise me on getting the last line of coverage, then I can update the documentation as well.

@ldesplat

This comment has been minimized.

Copy link
Contributor Author

ldesplat commented Feb 21, 2015

As far as the parameter is concerned, I'd rather take a path or a simple name, this would allow you to -T ./lab-babel.js for a local file or -T lab-label for a node module.

It's just a path. I do Path.resolve. I don't want to get into resolving between a path and a module.

@ldesplat

This comment has been minimized.

Copy link
Contributor Author

ldesplat commented Feb 24, 2015

I consider this PR ready unless there are any objections.

Now I am not making a great case for myself but another PR will have to submitted because I would like to pass an option to eslint (that is not possible to do over .eslintrc) and that is extensions in CLIEngine or --ext in cli. This allows eslint to read not just .js files but also other potential file extensions (ie. .js.es6 or .jsx) as it supports these features. Not an easy PR unfortunately since the linters are using ChildProcess.fork but I guess it could be passed as args.

Anyways, this is currently working great for my current project where I have some javascript code (with es6 features and jsx) running in both node and the browser and I am able to test it all with Lab pretty transparently.

README.md Outdated
@@ -43,6 +43,7 @@ global manipulation. Our goal with **lab** is to keep the execution engine as si
- `-s`, `--silence` - silence test output, defaults to false.
- `-S`, `--sourcemaps` - enables sourcemap support for stack traces and code coverage, disabled by default.
- `-t`, `--threshold` - minimum code test coverage percentage (sets `-c`), defaults to 100%.
- `-T`, `--transform` - javascript file that exports an array of objects ie. `[ { ext: ".js", transform: function (content) { ... } } ]`

This comment has been minimized.

Copy link
@geek

geek Feb 24, 2015

Member

Please add a note that this can't be used alongside coverage

@@ -277,5 +286,17 @@ internals.options = function () {
options.ids = argv.id;
}

if (options.transform) {

This comment has been minimized.

Copy link
@geek

geek Feb 24, 2015

Member

require hoek and assert on this option if it exists.

Hoek.assert(Array.isArray(transform), 'transform module must export an array of objects {ext: ".js", transform: null or function (content)}');

options.transform = transform;
return transform(content);
}

return content;

This comment has been minimized.

Copy link
@geek

geek Feb 24, 2015

Member

using a ternary here is fine instead of the above condition:

return (typeof transform === 'function') ? transform(content) : content;

exports.instrument = function (options) {

internals.patterns.unshift(internals.pattern(options));

if (options.transform) {

This comment has been minimized.

Copy link
@geek

geek Feb 24, 2015

Member

Might be better off doing Array.isArray(options.transform) since you are calling forEach on it.

@geek geek self-assigned this Feb 24, 2015
@geek geek added this to the 5.2.2 milestone Feb 24, 2015
@geek geek added the feature label Feb 24, 2015
@ldesplat

This comment has been minimized.

Copy link
Contributor Author

ldesplat commented Feb 25, 2015

Thank you @geek for the comments. I have made the changes.

geek added a commit that referenced this pull request Feb 26, 2015
Add live transform of source code before coverage analysis
@geek geek merged commit 3e7aade into hapijs:master Feb 26, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geek

This comment has been minimized.

Copy link
Member

geek commented Feb 26, 2015

@ldesplat awesome work, this will land in 5.3.0, which will be published today

@jedireza

This comment has been minimized.

Copy link
Contributor

jedireza commented Apr 7, 2015

@ldesplat we're seeing inconsistencies with line coverage with html generated reports, the command line coverage seems accurate though. Have you noticed/tested that?

@ldesplat

This comment has been minimized.

Copy link
Contributor Author

ldesplat commented Apr 7, 2015

@jedireza Expand your screen to the right. There are multiple columns on the table. You will see the right line numbers then.

But otherwise, yes the html report is not very nice. I had looked into generating a new report that assumes that transformed files are always the same (not 2 files into 1). This would simplify the report a lot and make it clear where the lines are and also could show the original code.

@jedireza

This comment has been minimized.

Copy link
Contributor

jedireza commented Apr 7, 2015

🙏 Thanks so much. It would be great if we could get the html highlighting on the right rows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.