Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

RC > Instrument different directory for libraries #448

Merged
merged 4 commits into from
Aug 9, 2018
Merged

RC > Instrument different directory for libraries #448

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cf3e2a8
Instrument different directory for libraries
Blackbaud-SteveBrush Aug 3, 2018
2b7a467
Logic adjustments
Blackbaud-SteveBrush Aug 9, 2018
bd9feb6
Removed comment
Blackbaud-SteveBrush Aug 9, 2018
ee5eb49
Merge branch 'rc-2.0.0' into rc-library-coverage
Blackbaud-SteveBrush Aug 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions config/webpack/test.webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,16 @@ function outPath() {
}

function getWebpackConfig(skyPagesConfig, argv) {
const runCoverage = (!argv || argv.coverage !== false);
skyPagesConfig.runtime.includeRouteModule = false;
const ENV = process.env.ENV = process.env.NODE_ENV = 'test';
const srcPath = path.resolve(process.cwd(), 'src', 'app');
const argvCoverage = (argv) ? argv.coverage : true;
const runCoverage = (argvCoverage !== false);

Copy link

Choose a reason for hiding this comment

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

This is probably more non-blocking feedback, but it stood out to me that we now have to check for argv twice. What are your thoughts on slightly refactoring to something like the following (untested)?

  const argvCoverage = argv ? argv.coverage : '';
  const srcPathUnresolved = [process.cwd(), 'src', 'app'];
  const runCoverage = argvCoverage !== false;
  
  if (argvCoverage === 'library') {
      srcPathUnresolved.push('public');
  }

  const srcPath = path.resolve(...srcPathUnresolved);
  skyPagesConfig.runtime.includeRouteModule = false;
  const ENV = process.env.ENV = process.env.NODE_ENV = 'test';

let srcPath;
if (argvCoverage === 'library') {
srcPath = path.resolve(process.cwd(), 'src', 'app', 'public');
} else {
srcPath = path.resolve(process.cwd(), 'src', 'app');
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@Blackbaud-BobbyEarl I agree about the argv.coverage checks! I updated the logic, there.

For the srcPath, I personally think writing out the paths long-hand for each condition is more readable, since the reader doesn't need to parse out the array.push behavior for each condition. Also, keeping them separate allows more flexibility if we want to change the paths in the future (for example, if the library's source path wasn't a descendant of the SPA's source path). Just my opinion. Thoughts on that?


const resolves = [
process.cwd(),
Expand All @@ -33,6 +39,8 @@ function getWebpackConfig(skyPagesConfig, argv) {
outPath('node_modules')
];

skyPagesConfig.runtime.includeRouteModule = false;

let alias = aliasBuilder.buildAliasList(skyPagesConfig);

let config = {
Expand Down
15 changes: 15 additions & 0 deletions test/config-webpack-test.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/*jshint jasmine: true, node: true */
'use strict';

const path = require('path');
const runtimeUtils = require('../utils/runtime-test-utils');

describe('config webpack test', () => {
Expand Down Expand Up @@ -104,4 +105,18 @@ describe('config webpack test', () => {
expect(foundMatch).toBe(true);
}
});

it('should run coverage differently for libraries', () => {
let instrumentLoader = getInstrumentLoader();
let index = instrumentLoader.include.indexOf(path.resolve('src', 'app'));

expect(index > -1).toEqual(true);

instrumentLoader = getInstrumentLoader({
coverage: 'library'
});

index = instrumentLoader.include.indexOf(path.resolve('src', 'app', 'public'));
expect(index > -1).toEqual(true);
});
});