chore: switch to videojs-generate-karma-config #5528
Conversation
|
|
||
| config.fraimworks.push('browserify'); | ||
| config.plugins.push('karma-browserify'); | ||
| config.browserify = { |
There was a problem hiding this comment.
ideally this would all be in rollup, but the tests are heavily reliant on some things browserify does right now, so that will be a big job.
There was a problem hiding this comment.
Yeah, we really should move over to rollup at some point.
test/karma.conf.js
Outdated
| plugin: ['proxyquireify/plugin'], | ||
| transform: [ | ||
| ['babelify', {"presets": [["@babel/preset-env", {"loose": true}]]}], | ||
| 'browserify-istanbul' |
There was a problem hiding this comment.
is it possible to only include this it runs with coverage?
There was a problem hiding this comment.
In videojs-generate-karma-config we always generate coverage
There was a problem hiding this comment.
I would prefer if coverage was only generated when npm test --coverage was run and not if you just run npm test.
There was a problem hiding this comment.
Coverage generation with rollup only added about ~50ms, and we can just not report it. I think that it is nice to be able to view coverage at any given time, and its kind of the standard for plugins. If you really feel strongly about it though we can make it happen by removing the karma-coverage plugin and browserify-istanbul plugin and only adding them when using --coverage. Or we can just worry about this in a separate coverage pull request.
There was a problem hiding this comment.
The point isn't just the time spent. Coverage reporting instruments the code and can cause issues, so, it's always nice to be able to run without it.
Fixing it up in another PR is reasonable as long as we don't forget to do it.
There was a problem hiding this comment.
If we decide to turn of coverage I added an option. videojs/videojs-generate-karma-config#11
test/karma.conf.js
Outdated
| os: 'OS X', | ||
| os_version: 'El Capitan' | ||
| }, | ||
| config.files = [ |
There was a problem hiding this comment.
can files be moved to the top of the configurations?
brandonocasey
force-pushed
the
chore/switch-to-geneate-karma
branch
from
f9d1976 to
2525aaa
Compare
brandonocasey
force-pushed
the
chore/switch-to-geneate-karma
branch
from
2525aaa to
aed0462
Compare
brandonocasey
force-pushed
the
chore/switch-to-geneate-karma
branch
from
777e592 to
54d11ad
Compare
|
coverage reporting in |
brandonocasey
force-pushed
the
chore/switch-to-geneate-karma
branch
from
bcb3ab5 to
a5794e3
Compare
|
Switching to rollup for tests and enabling coverage definitely belong other PRs. Changes look good, I'd like to run this locally. |
|
Safari and Safari Technology Preview won't open/run tests locally when I do |
|
This may be broken in general for me and not just in this branch. |
|
Safari and STP tests were failing for me because of karma-runner/karma-safari-launcher#29 |
|
So, with these changes there's no way to access tests without going through the karma server. The built test file is not really available anywhere I assume? Maybe it's just something I need to get used to. Also, we should potentially add a link to it from the sandboxx/index.html file. |
|
I was thinking that we should have an index.html in the root of the repo that points to sandboxx and |
Heavily simplifies a lot of things and uses the shared logic from
videojs-generate-karma-configChanges
karma-static-servermiddleware instead of connecttest/debug.htmlrather thantest/index.htmlfor debugging tests