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

Karma support #306

Merged
merged 10 commits into from
Jan 25, 2019
Merged

Karma support #306

merged 10 commits into from
Jan 25, 2019

Conversation

rjatkins
Copy link
Contributor

Implements #188

 * Currently requires installing a whole mess of karma plugins locally to verify. Next step is to use a node require mocking framework to avoid this
 * Track optional dependencies in the repo, across platforms
@codecov
Copy link

codecov bot commented Jan 23, 2019

Codecov Report

Merging #306 into master will decrease coverage by 0.31%.
The diff coverage is 95.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #306      +/-   ##
==========================================
- Coverage   98.05%   97.74%   -0.32%     
==========================================
  Files          30       31       +1     
  Lines         462      531      +69     
==========================================
+ Hits          453      519      +66     
- Misses          9       12       +3
Impacted Files Coverage Δ
src/special/karma.js 95.65% <95.65%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a09a06...b3cca8c. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 23, 2019

Codecov Report

Merging #306 into master will increase coverage by 0.28%.
The diff coverage is 99.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #306      +/-   ##
==========================================
+ Coverage   98.11%   98.39%   +0.28%     
==========================================
  Files          30       32       +2     
  Lines         477      561      +84     
==========================================
+ Hits          468      552      +84     
  Misses          9        9
Impacted Files Coverage Δ
src/check.js 100% <ø> (+1.16%) ⬆️
src/utils/linters.js 100% <ø> (ø) ⬆️
src/utils/index.js 93.75% <100%> (+6.25%) ⬆️
src/special/karma.js 100% <100%> (ø)
src/utils/parser.js 93.75% <93.75%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1375cf2...16c33a7. Read the comment docs.

const frameworkMapping = {};
// skip plugins with more specific names, such as launchers and reporters
karmaPluginsInstalled.filter(plugin => !plugin.endsWith('-launcher') && !plugin.endsWith('-reporter')).forEach((genericPlugin) => {
const p = tryRequire(genericPlugin);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This specific approach - using node's require to try to pull in the relevant module - seems to be against the philosophy of the rest of the project. Sadly the information we need to resolve frameworks/browsers/reporters/etc to plugins isn't quite available statically, which means we need to find the default module in each plugin and parse out the map from its module.export. Any guidance on how to do that cheaply would be much appreciated!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opted to replace it with parser code, so I can guarantee it works in my own project.

…m instead

 * node require is not looking at the modules of the project under test, so we have to find the plugin metadata ourselves
 * add resolve@1.10.0 as a dependency so this can work
 * restructured karma special and its test to work with these new dependencies. Good news: no need for proxyquire to inject globals now
@rumpl
Copy link
Member

rumpl commented Jan 25, 2019

The build fails on node 6, I'm not against dropping node 6 support...

 * Supports only a simplified use of karma.conf.js where the caller passes a literal object expression, with string literals for keys and values in all supported config properties
 * Avoids problems loading node dependencies for config or plugin modules, while still extracting the important parts of the relevant files
 * Improves backwards compatibility with node 6 on appveyor, until we get an updated version of @babel/polyfill working
@rjatkins
Copy link
Contributor Author

The build fails on node 6, I'm not against dropping node 6 support...

It's easy enough to stop using Object.values and make it backwards compatible again, though I'm really confused why @babel/polyfill isn't triggering in appveyor, but works fine in travis. Doing that instead.

 * Turns out I never needed the keys in this call anyway
@rumpl rumpl merged commit 69005fb into depcheck:master Jan 25, 2019
@rumpl
Copy link
Member

rumpl commented Jan 25, 2019

🎉

Thanks!

@rjatkins rjatkins deleted the karma-support branch October 21, 2019 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants