docs(recipe): Add comments for precompiling multiple test files #1385
Conversation
There was a problem hiding this comment.
Thanks for the PR @sudo-suhas. I've left some inline comments.
| target: 'node', | ||
| output: { | ||
| path: '_build', | ||
| path: path.resolve(__dirname, '_build'), |
There was a problem hiding this comment.
I'm not sure the sample code needs to prescribe this. What does webpack show in its examples?
There was a problem hiding this comment.
According to the webpack docs, the target directory for all output files must be an absolute path (use the Node.js path module). You can see this in the code example here - https://webpack.js.org/configuration/
| const nodeExternals = require('webpack-node-externals'); | ||
|
|
||
| module.exports = { | ||
| entry: ['src/tests.js'], | ||
| // entry: glob.sync('./test/**/*.js'), |
There was a problem hiding this comment.
The recipe discusses precompiling source files. I'm not sure why the entry here is src/tests.js but it seems even more incongruent to add test/**/*.js entries.
How are you using this?
Perhaps the globbing approach can be mentioned in the paragraph that follows the example code, with a link to the Stack Overflow post?
There was a problem hiding this comment.
I'm not sure why the entry here is
src/tests.js
Perhaps the entry should be changed to tests/index.js?
How are you using this?
I am using this in elastic-builder. You can see the improved test duration here - https://travis-ci.org/sudo-suhas/elastic-builder/builds.
Perhaps the globbing approach can be mentioned in the paragraph that follows the example code, with a link to the Stack Overflow post?
I wasn't entirely sure about how to present it. I am okay with this. Shall I push a commit for this?
There was a problem hiding this comment.
The entry is the test files, because they in turn include the source files that need to be precompiled. No reason precompiling files that aren't ever tested.
There was a problem hiding this comment.
Also, the bundle in this case will be the test files and all the source files that are included in those tests.
There was a problem hiding this comment.
@sudo-suhas Also, am I reading that CI output correctly? 31 min 39 sec to 5 min 49 sec!? Wow!
There was a problem hiding this comment.
Not exactly.. It is the combined time for execution in 3 node envs(in parallel) .. But the test duration did go down from nearly 13 mins of 100% resource usage to under 3 mins in one run.
Do you think it would be more appropriate for this recipe to discuss multiple test files transpilation by default? I understand it can also be used for cases where you have aliases too. But to me it seems like the former is the more prominent use case.
There was a problem hiding this comment.
Yeah, I think this is a good changeset. It's far more common to have a bunch of test files that you would want to glob for.
|
What confuses me is that the recipe talks about precompiling source files, but then the example precompiles a test file, which is how you're using it. @danny-andrews what was the original intent? I can't quite decide how this is meant to be used based on #1212 and #1011. @sudo-suhas I think we need to clear this up (might just be that the recipe title is misleading). But then yes let's add the globbing approach in the paragraph after the example. |
|
@novemberborn I could be wrong, but my understanding is, it is precompiling source files. Only indirectly. So in my case, I had many independent test files. And had to use |
|
Thanks for the clarification @danny-andrews. @sudo-suhas I think you're right that this recipe could use globbing as standard. Should it output a file for each entry though? Otherwise all tests run in the same process. |
|
@novemberborn If I wanted to output multiple files, how would I do it? Take the basename for each file? Also, if it performs well, do we care if the tests run in the same process? |
|
I haven't used webpack that much, so I don't know the exact syntax, but I think it should be possible. It can matter if tests modify globals Node.js built-ins. You wouldn't want that to leak across tests. There's a lot of nuance here, I'd be happy if we can clarify the specific use cases for which this approach is safe. |
|
@novemberborn Does ava parallelize tests across files or |
|
@danny-andrews both. Each file runs in its own process, configurably in parallel. And all test are started at the same time, so asynchronous tests also run in parallel. |
|
I'll put together a sample github repo sometime this week, so we can play around with the webpack config and demonstrate an example use-case. |
|
The following webpack config outputs a file for each entry: 'use strict';
const path = require('path');
const glob = require('glob');
const nodeExternals = require('webpack-node-externals');
const testFiles = glob.sync('./test/**/*.js');
const entryObj = {};
for (let idx = 0; idx < testFiles.length; idx++) {
const file = testFiles[idx];
entryObj[path.basename(file, path.extname(file))] = file;
}
module.exports = {
target: 'node',
entry: entryObj,
output: {
path: path.resolve(__dirname, '_build'),
filename: '[name].js'
},
externals: [nodeExternals()],
module: {
rules: [
{
test: /\.(js|jsx)$/,
use: {
loader: 'babel-loader',
// Adding this saved ~9 seconds for me
options: {
cacheDirectory: true
}
}
}
]
}
};But this negates any performance benefit because each entry has its own copy of the src. If I add the commons plugin: plugins: [
new webpack.optimize.CommonsChunkPlugin({
name: 'commons',
filename: 'commons.js',
minChunks: 3
})
]Every test file fails: Specifying entry for src and using that in the plugin doesn't help either. |
Starting the worker processes may still be faster, even if the source is duplicated on disk. |
|
Not entirely sure which part of the setup is responsible for this but there is a huge difference in execution time between 1st(no cache) and subsequent runs. Here's the test run timings:
Webpack output(single entry)Webpack output(multiple entries) |
|
The 1st/second run difference implies it might be because AVA precompiles the test file, which is much bigger with the webpack build. If you only run one file (no concurrency) you only pay that cost once, but with multiple files you end up paying it for each file. |
|
@novemberborn I am a little confused. What do you mean by AVA precompiles the test file? Isn't that what we are doing before feeding it to AVA? |
|
@sudo-suhas it looks like you're precompiling and bundling the source files. You then run the resulting bundle with AVA, which treats the entire bundle as a test file. Unless you disable it (which may not actually be entirely possible, can't recall at the moment) it'll then transpile the test file. See https://github.com/avajs/ava/blob/master/docs/recipes/babelrc.md for more. |
|
From the README
I don't think there is a way to tell AVA that there is no need to transpile again even if we were to apply those transforms in out precompile step. ~17m is not acceptable even though it'll be once. Are there any other options we can consider? |
|
@sudo-suhas for your use case I suppose it's better to build everything into one file. Not sure where that leaves us with this recipe though. |
|
Maybe we should have this recipe recommend building everything to one file, but add a note explaining that all tests will be run in the same process. Also, this use-case makes me think that there should be an option to opt-out of ava's auto test transformation. @sindresorhus what are your thoughts on this? |
|
I have a solution for this but it is not pretty. Let me know what you think. Comments inline. // npm scripts
{
"scripts": {
// Use the babel cli to compile the src files but preserve file structure
"precompile-src": "cross-env NODE_ENV=test babel src --out-dir _src",
// We still need to compile tests to change the require path for src files
"precompile-tests": "cross-env NODE_ENV=test webpack --config webpack.config.test.js",
"pretest": "npm run precompile-src && npm run precompile-tests",
"test": "cross-env NODE_ENV=test nyc --cache ava _build --concurrency 3"
}
}Webpack Externals Docs - https://webpack.js.org/configuration/externals/#function webpack.config.test.js 'use strict';
const path = require('path');
const glob = require('glob');
const nodeExternals = require('webpack-node-externals');
const testFiles = glob.sync('./test/**/*.js');
const entryObj = {};
for (let idx = 0; idx < testFiles.length; idx++) {
const file = testFiles[idx];
entryObj[path.basename(file, path.extname(file))] = file;
}
module.exports = {
target: 'node',
entry: entryObj,
output: {
path: path.resolve(__dirname, '_build'),
filename: '[name].js'
},
externals: [
nodeExternals(),
// Rewrite the require paths to use _src
(context, request, callback) => {
// This is a little messy because tests are not output in original file structure
// test/index.test.js -> _build/index.test.js
// => ../src -> ../_src
// test/aggregations-test/avg-agg.test.js -> _build/avg-agg.test.js
// => ../../src -> ../_src
if (request.includes('/src')) {
const requestReqwrite = request
.replace('/src', '/_src')
.replace('../../_src', '../_src');
return callback(null, `commonjs ${requestReqwrite}`);
}
callback();
}
]
};Webpack outputCompiled test file sampleOriginal test fileimport test from 'ava';
import { AvgAggregation } from '../../src';
import { setsAggType } from '../_macros';
test(setsAggType, AvgAggregation, 'avg');
test('constructor sets field', t => {
const value = new AvgAggregation('my_agg', 'my_field').toJSON();
const expected = {
my_agg: {
avg: {
field: 'my_field'
}
}
};
t.deepEqual(value, expected);
});Compiled file/******/ (function(modules) { // webpackBootstrap
/******/ // The module cache
/******/ var installedModules = {};
/******/
/******/ // The require function
/******/ function __webpack_require__(moduleId) {
/******/
/******/ // Check if module is in cache
/******/ if(installedModules[moduleId]) {
/******/ return installedModules[moduleId].exports;
/******/ }
/******/ // Create a new module (and put it into the cache)
/******/ var module = installedModules[moduleId] = {
/******/ i: moduleId,
/******/ l: false,
/******/ exports: {}
/******/ };
/******/
/******/ // Execute the module function
/******/ modules[moduleId].call(module.exports, module, module.exports, __webpack_require__);
/******/
/******/ // Flag the module as loaded
/******/ module.l = true;
/******/
/******/ // Return the exports of the module
/******/ return module.exports;
/******/ }
/******/
/******/
/******/ // expose the modules object (__webpack_modules__)
/******/ __webpack_require__.m = modules;
/******/
/******/ // expose the module cache
/******/ __webpack_require__.c = installedModules;
/******/
/******/ // identity function for calling harmony imports with the correct context
/******/ __webpack_require__.i = function(value) { return value; };
/******/
/******/ // define getter function for harmony exports
/******/ __webpack_require__.d = function(exports, name, getter) {
/******/ if(!__webpack_require__.o(exports, name)) {
/******/ Object.defineProperty(exports, name, {
/******/ configurable: false,
/******/ enumerable: true,
/******/ get: getter
/******/ });
/******/ }
/******/ };
/******/
/******/ // getDefaultExport function for compatibility with non-harmony modules
/******/ __webpack_require__.n = function(module) {
/******/ var getter = module && module.__esModule ?
/******/ function getDefault() { return module['default']; } :
/******/ function getModuleExports() { return module; };
/******/ __webpack_require__.d(getter, 'a', getter);
/******/ return getter;
/******/ };
/******/
/******/ // Object.prototype.hasOwnProperty.call
/******/ __webpack_require__.o = function(object, property) { return Object.prototype.hasOwnProperty.call(object, property); };
/******/
/******/ // __webpack_public_path__
/******/ __webpack_require__.p = "";
/******/
/******/ // Load entry module and return exports
/******/ return __webpack_require__(__webpack_require__.s = 15);
/******/ })
/************************************************************************/
/******/ ([
/* 0 */
/***/ (function(module, exports) {
module.exports = require("ava");
/***/ }),
/* 1 */
/***/ (function(module, exports) {
module.exports = require("../_src");
/***/ }),
/* 2 */
/***/ (function(module, __webpack_exports__, __webpack_require__) {
"use strict";
Object.defineProperty(__webpack_exports__, "__esModule", { value: true });
/* harmony export (immutable) */ __webpack_exports__["setsAggType"] = setsAggType;
/* harmony export (immutable) */ __webpack_exports__["illegalCall"] = illegalCall;
/* harmony export (immutable) */ __webpack_exports__["illegalParamType"] = illegalParamType;
/* harmony export (immutable) */ __webpack_exports__["validatedCorrectly"] = validatedCorrectly;
/* harmony export (immutable) */ __webpack_exports__["simpleExpect"] = simpleExpect;
/* harmony export (immutable) */ __webpack_exports__["aggsExpectStrategy"] = aggsExpectStrategy;
/* harmony export (immutable) */ __webpack_exports__["nameExpectStrategy"] = nameExpectStrategy;
/* harmony export (immutable) */ __webpack_exports__["nameFieldExpectStrategy"] = nameFieldExpectStrategy;
/* harmony export (immutable) */ __webpack_exports__["makeSetsOptionMacro"] = makeSetsOptionMacro;
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_0_lodash__ = __webpack_require__(4);
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_0_lodash___default = __webpack_require__.n(__WEBPACK_IMPORTED_MODULE_0_lodash__);
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_1__src_core_util__ = __webpack_require__(3);
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_1__src_core_util___default = __webpack_require__.n(__WEBPACK_IMPORTED_MODULE_1__src_core_util__);
const ILLEGAL_PARAM = Object.create(null);
/**
* Macro for testing that aggregation type is set as expected.
*
* @param {*} t
* @param {function} Cls
* @param {string} aggType
* @param {Object=} defaultDef
*/
function setsAggType(t, Cls, aggType, defaultDef) {
const value = new Cls('my_agg').toJSON();
const expected = {
my_agg: {
[aggType]: Object.assign({}, defaultDef)
}
};
t.deepEqual(value, expected);
}
setsAggType.title = (ignore, Cls, aggType) => `sets type as ${aggType}`;
/**
* Macro for checking method cannot be called on the instance
*
* @param {*} t
* @param {*} Cls constructor class
* @param {string} propKey method name
*/
function illegalCall(t, Cls, propKey, ...args) {
const err = t.throws(() => new Cls(...args)[propKey](), Error);
t.is(err.message, `${propKey} is not supported in ${Cls.name}`);
}
illegalCall.title = (ignore, Cls, propKey) => `${__WEBPACK_IMPORTED_MODULE_0_lodash___default.a.snakeCase(propKey)} cannot be set`;
/**
* Check that calling method on instance with illegal param type throws error
*
* @param {*} t
* @param {*} instance
* @param {string} method
* @param {string} clsName
*/
function illegalParamType(t, instance, method, clsName) {
let err = t.throws(() => instance[method](null), TypeError);
t.is(err.message, `Argument must be an instance of ${clsName}`);
err = t.throws(() => instance[method](ILLEGAL_PARAM), TypeError);
t.is(err.message, `Argument must be an instance of ${clsName}`);
}
illegalParamType.title = (ignore, instance, method, clsName) => `checks ${clsName} class`;
/**
* Macro for testing method validation
*
* @param {*} t
* @param {function} getInstance
* @param {string} method
* @param {Array} validValues
* @param {boolean=} toggleCase
*/
function validatedCorrectly(t, getInstance, method, validValues, toggleCase = true) {
__WEBPACK_IMPORTED_MODULE_0_lodash___default.a.forEach(validValues, val => {
t.notThrows(() => getInstance()[method](val));
if (toggleCase) {
t.notThrows(() => getInstance()[method](val.toLowerCase()));
t.notThrows(() => getInstance()[method](val.toUpperCase()));
}
});
t.throws(() => getInstance()[method](null));
t.throws(() => getInstance()[method](`invalid_${__WEBPACK_IMPORTED_MODULE_0_lodash___default.a.snakeCase(method)}`));
}
validatedCorrectly.title = (ignore, getInstance, method) =>
`${__WEBPACK_IMPORTED_MODULE_0_lodash___default.a.snakeCase(method)} correctly validated`;
/**
* Simple strategy for checking option is set for use with `makeSetsOptionMacro`
*
* @param {string} keyName
* @param {*} propValue
* @returns {function}
*/
function simpleExpect(keyName, propValue) {
return { [keyName]: propValue };
}
/**
* Expect strategy for use with `makeSetsOptionMacro` for aggregations
*
* @param {string} name
* @param {string} type
* @param {Object} defaultDef
* @returns {function}
*/
function aggsExpectStrategy(name, type, defaultDef) {
return (keyName, propValue) => ({
[name]: {
[type]: Object.assign({ [keyName]: propValue }, defaultDef)
}
});
}
/**
* Expect strategy for use with `makeSetsOptionMacro` for queries, score functions
*
* @param {string} name
* @param {Object=} defaultDef
* @returns {function}
*/
function nameExpectStrategy(name, defaultDef) {
return (keyName, propValue) => ({
[name]: Object.assign({}, defaultDef, { [keyName]: propValue })
});
}
/**
* Expect strategy for use with `makeSetsOptionMacro` for full text queries
*
* @param {string} name
* @param {Object=} defaultDef
* @returns {function}
*/
function nameFieldExpectStrategy(name, defaultDef) {
return (keyName, propValue) => ({
[name]: {
my_field: Object.assign({}, defaultDef, { [keyName]: propValue })
}
});
}
/**
* Make macro for checking property is set.
*
* @param {function} getInstance
* @param {function=} getExpected Set to `simpleExpect` by default
* @returns {function}
*/
function makeSetsOptionMacro(getInstance, getExpected = simpleExpect) {
/**
* Macro for testing that property is being set
*
* @param {*} t
* @param {string} methodName
* @param {Object} options
* @param {*} options.param
* @param {*=} options.propValue Optional argument for use when value passed is not the value set
* @param {boolean=} options.spread If array is passed, to control spread
* @param {string=} options.keyName Optional override argument, default is `_.snakeCase(methodName)`
*/
function setsOption(
t,
methodName,
{ param, propValue = param, spread = true, keyName = __WEBPACK_IMPORTED_MODULE_0_lodash___default.a.snakeCase(methodName) }
) {
const value = Array.isArray(param) && spread
? getInstance()[methodName](...param).toJSON()
: getInstance()[methodName](param).toJSON();
const expected = getExpected(keyName, __webpack_require__.i(__WEBPACK_IMPORTED_MODULE_1__src_core_util__["recursiveToJSON"])(propValue));
t.deepEqual(value, expected);
}
setsOption.title = (providedTitle, methodName) =>
!__WEBPACK_IMPORTED_MODULE_0_lodash___default.a.isEmpty(providedTitle) ? providedTitle : `sets ${__WEBPACK_IMPORTED_MODULE_0_lodash___default.a.snakeCase(methodName)} option`;
return setsOption;
}
/***/ }),
/* 3 */
/***/ (function(module, exports) {
module.exports = require("../_src/core/util");
/***/ }),
/* 4 */
/***/ (function(module, exports) {
module.exports = require("lodash");
/***/ }),
/* 5 */,
/* 6 */,
/* 7 */,
/* 8 */,
/* 9 */,
/* 10 */,
/* 11 */,
/* 12 */,
/* 13 */,
/* 14 */,
/* 15 */
/***/ (function(module, __webpack_exports__, __webpack_require__) {
"use strict";
Object.defineProperty(__webpack_exports__, "__esModule", { value: true });
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_0_ava__ = __webpack_require__(0);
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_0_ava___default = __webpack_require__.n(__WEBPACK_IMPORTED_MODULE_0_ava__);
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_1__src__ = __webpack_require__(1);
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_1__src___default = __webpack_require__.n(__WEBPACK_IMPORTED_MODULE_1__src__);
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_2__macros__ = __webpack_require__(2);
__WEBPACK_IMPORTED_MODULE_0_ava___default()(__WEBPACK_IMPORTED_MODULE_2__macros__["setsAggType"], __WEBPACK_IMPORTED_MODULE_1__src__["AvgAggregation"], 'avg');
__WEBPACK_IMPORTED_MODULE_0_ava___default()('constructor sets field', t => {
const value = new __WEBPACK_IMPORTED_MODULE_1__src__["AvgAggregation"]('my_agg', 'my_field').toJSON();
const expected = {
my_agg: {
avg: {
field: 'my_field'
}
}
};
t.deepEqual(value, expected);
});
/***/ })
/******/ ]);1st run time - Performance wise, single entry test file is the best. Although this is too complicated to setup and introduces too many restrictions on file structure, |
@danny-andrews that makes sense. @sudo-suhas?
There's a plan for that. Eventually we may be able to integrate webpack in the same way. |
@danny-andrews, @novemberborn: there is a huge downside on this approach: we lose the name of the tests outputted when in verbose mode. Because ava use the name and path of the file to output this on verbose mode. |
@novemberborn Initially I did agree with the suggestion. However, I am not so sure now. All the various approaches we have discussed come with their own but. However, this discussion itself can be very useful for somebody trying to setup precompilation. So why not add a paragraph summarising the discussion and linking back to this thread? |
|
@sudo-suhas I agree. There is a lot of nuance surrounding this setup. It might be better to list the different approaches discussed here along with the pros and cons of each without explicitly recommending one over the other, as there doesn't seem to be one best solution as of yet. When this is completed, this may no longer be the case. |
|
@sudo-suhas sounds good 👍 |
sudo-suhas
force-pushed
the
docs_precompile_multi
branch
from
fb90bf3 to
6bd0497
Compare
|
I have updated the PR but it might be a bit too long. Let me know if you think we can drop any of the discussed approaches or trim anything. Markdown link |
There was a problem hiding this comment.
This is great @sudo-suhas, thank you!
I've left some comments on sentence structure and some typos, as well as some code questions. Excited to see this land 👍
| @@ -2,25 +2,33 @@ | |||
|
|
|||
| Translations: [Français](https://github.com/avajs/ava-docs/blob/master/fr_FR/docs/recipes/precompiling-with-webpack.md) | |||
|
|
|||
| The AVA [readme](https://github.com/avajs/ava#transpiling-imported-modules) mentions precompiling your imported modules as an alternative to runtime compilation, but it doesn't explain how. This recipe shows how to do this using webpack. (This example uses webpack 2.0) | |||
| The AVA [readme](https://github.com/avajs/ava#transpiling-imported-modules) mentions precompiling your imported modules as an alternative to runtime compilation, but it doesn't explain how. This recipe various approaches using webpack. (These examples use webpack 2.0). Multiple approaches are discussed as each has its own pros and cons. You should select the approach that best fits your use case. This might not be necessery once [this](https://github.com/avajs/ava/blob/master/docs/specs/001%20-%20Improving%20language%20support.md) is completed. See the original discussion [here](/avajs/ava/pull/1385). | |||
There was a problem hiding this comment.
Rather than:
This recipe various approaches using webpack. (These examples use webpack 2.0).
Perhaps:
This recipe discusses several approaches using webpack v2.
This seems distracting:
This might not be necessery once this is completed.
And the discussion URL should have https://github.com in front of it.
There was a problem hiding this comment.
This seems distracting:
Thought it would be better to acknowledge the difficulty of precompiling and mention that a better permanent solution is in the works. Would you prefer I reword it or remove it?
And the discussion URL should have https://github.com in front of it.
I thought it might be better to have relative links. And that / would go down to the root. I'll just change this to use full URL. There is also the option of using avajs/ava#1385. Let me know if you'd prefer it.
There was a problem hiding this comment.
Thought it would be better to acknowledge the difficulty of precompiling and mention that a better permanent solution is in the works. Would you prefer I reword it or remove it?
I'd rather these recipes focus on what's possible now, rather than what might be possible eventually. I mean I wrote that RFC in January… it's not a quick fix 😉
I thought it might be better to have relative links. And that / would go down to the root. I'll just change this to use full URL. There is also the option of using #1385. Let me know if you'd prefer it.
Yea I know. I suppose I just don't trust linking within GitHub that much, plus it means the links won't work if the file is rendered outside of GitHub.
| const nodeExternals = require('webpack-node-externals'); | ||
|
|
||
| module.exports = { | ||
| entry: ['src/tests.js'], | ||
| entry: ['tests.js'], |
There was a problem hiding this comment.
This assumes you only have one test file (named tests.js)? Perhaps the file name should be test.js then? (At least that's how I name such a file, analogous to having a test directory.)
There was a problem hiding this comment.
Although there is a single file that's not the same thing as having a single test. I don't mind changing this though.
There was a problem hiding this comment.
I hear ya. We do talk about test.js in the readme though.
Alternatively: your-test-file.js?
There was a problem hiding this comment.
Let's go with test.js. Sold on the readme.
| - [Multiple entry files](#multiple-entry-files) | ||
| - [Multiple entry files with externalized source files](#multiple-entry-files-with-externalized-source-files) | ||
|
|
||
| #### Refer precompiled source in tests |
There was a problem hiding this comment.
Perhaps Test against precompiled sources?
| - [Multiple entry files with externalized source files](#multiple-entry-files-with-externalized-source-files) | ||
|
|
||
| #### Refer precompiled source in tests | ||
| Source files can be compiled with source maps(for coverage) to `_src` folder and referenced in tests. While this is less than elegant, it performs well and the worklow can be optimized with [webpack's watch mode](https://webpack.js.org/configuration/watch/). |
There was a problem hiding this comment.
Let's not bring code coverage into this discussion, since that's a whole other can of worms.
Why _src here versus _build above?
There's a typo in workflow (it's missing the l).
How would users do this precompilation?
There was a problem hiding this comment.
Why
_srchere versus_buildabove?
I was thinking _build for tests and _src for source.
How would users do this precompilation?
I was thinking babel-cli.. I'll change this to mention babel-cli watch mode instead of webpack. Sound good?
There was a problem hiding this comment.
I was thinking _build for tests and _src for source.
Sure. I'm probably overthinking this 😄
How would users do this precompilation?
I was thinking babel-cli.. I'll change this to mention babel-cli watch mode instead of webpack. Sound good?
👍
| ``` | ||
|
|
||
| #### Single entry file | ||
| To pre-compile folder with multiple test files, we can use globbing(see [stackoverflow answer](http://stackoverflow.com/questions/32874025/how-to-add-wildcard-mapping-in-entry-of-webpack/34545812#34545812)). Although this performs quite possibly the best, it comes at a cost. AVA uses the filename and path for test names in verbose mode as well as test failure reports. This can make the error report harder to understand. Also, it can matter if tests modify globals, Node.js built-ins. You wouldn't want that to leak across tests. |
There was a problem hiding this comment.
How about putting it like this?
Multiple test files can be compiled into a single file. This may have the best performance but that does come at a cost. All tests will be in the same file, which can make it harder to know which test has failed, since AVA can't show the file name the test was originally in. You'll also lose process isolation.
| for (let idx = 0; idx < len; idx++) { | ||
| const file = testFiles[idx]; | ||
| entryObj[path.basename(file, path.extname(file))] = file; | ||
| } |
There was a problem hiding this comment.
This code can be improved a bit:
const testFiles = glob.sync('./test/**/*.js').reduce((entryObj, file) => {
entryObj[path.basename(file, path.extname(file))] = file;
return entryObj;
}, {});Then use entry: testFiles below.
There was a problem hiding this comment.
I too used reduce initially but felt the for method was easier to understand. The reduce returns the entryObj and not the testFiles though. I'll change it to use reduce.
| ``` | ||
|
|
||
| #### Multiple entry files with externalized source files | ||
| This is the most complicated to setup but performs quite well and also retains file names. In this approach, we use the babel cli to compile the src files but preserve file structure. Then we compile the tests with a require path rewrite. The following example is for a specific file structure. Depending on how your source and test files are organised, you might need to make changes. |
There was a problem hiding this comment.
I'd replace "babel cli" with babel-cli. Rather than "compile the src file" I'd use "compile the source files". And I'd invert the sentence structure from:
Then we compile the tests with a require path rewrite"
To:
Require paths in tests are rewritten when compiling them in webpack.
| for (let idx = 0; idx < len; idx++) { | ||
| const file = testFiles[idx]; | ||
| entryObj[path.basename(file, path.extname(file))] = file; | ||
| } |
|
@novemberborn Thanks for taking the time to review. |
|
Landed. Thanks @sudo-suhas for your work on this, and thank you @danny-andrews and @grvcoelho for your contributions in the discussion. |
Discuss various options and trade-offs in how to use webpack builds as a way to run AVA tests.
Adds comments for pre-compilation of multiple test files which ava supports. Also changed the target output directory to absolute path in accordance with webpack docs