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

chore: switch to videojs-generate-karma-config #5528

Merged
merged 10 commits into from Oct 31, 2018

Conversation

brandonocasey
Copy link
Contributor

@brandonocasey brandonocasey commented Oct 25, 2018

Heavily simplifies a lot of things and uses the shared logic from videojs-generate-karma-config

Changes

  • Removes all browserify stuff outside of karma
  • Uses karma-static-server middleware instead of connect
  • Uses test/debug.html rather than test/index.html for debugging tests


config.frameworks.push('browserify');
config.plugins.push('karma-browserify');
config.browserify = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we really should move over to rollup at some point.

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

coverage stuff should probably be done in a separate PR

test/karma.conf.js Show resolved Hide resolved
test/karma.conf.js Show resolved Hide resolved
test/karma.conf.js Show resolved Hide resolved
plugin: ['proxyquireify/plugin'],
transform: [
['babelify', {"presets": [["@babel/preset-env", {"loose": true}]]}],
'browserify-istanbul'
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to only include this it runs with coverage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In videojs-generate-karma-config we always generate coverage

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if coverage was only generated when npm test --coverage was run and not if you just run npm test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we decide to turn of coverage I added an option. videojs/videojs-generate-karma-config#11

os: 'OS X',
os_version: 'El Capitan'
},
config.files = [
Copy link
Member

Choose a reason for hiding this comment

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

can files be moved to the top of the configurations?

test/karma.conf.js Show resolved Hide resolved
build/grunt.js Show resolved Hide resolved
build/grunt.js Show resolved Hide resolved
@brandonocasey
Copy link
Contributor Author

coverage reporting in karma-coverage is throwing an error, I think we should opt to fix it in another pr, but coverage generation appears to be working.

@gkatsev
Copy link
Member

gkatsev commented Oct 29, 2018

Switching to rollup for tests and enabling coverage definitely belong other PRs. Changes look good, I'd like to run this locally.

@gkatsev
Copy link
Member

gkatsev commented Oct 29, 2018

Safari and Safari Technology Preview won't open/run tests locally when I do npm test.

@gkatsev
Copy link
Member

gkatsev commented Oct 29, 2018

This may be broken in general for me and not just in this branch.

@gkatsev
Copy link
Member

gkatsev commented Oct 29, 2018

Safari and STP tests were failing for me because of karma-runner/karma-safari-launcher#29

@gkatsev
Copy link
Member

gkatsev commented Oct 29, 2018

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 sandbox/index.html file.

@brandonocasey
Copy link
Contributor Author

I was thinking that we should have an index.html in the root of the repo that points to sandbox and /test/debug.html which is similar to our previous page that does not run tests through karma

@gkatsev gkatsev merged commit 2e70450 into master Oct 31, 2018
@gkatsev gkatsev deleted the chore/switch-to-geneate-karma branch October 31, 2018 15:01
@brandonocasey brandonocasey mentioned this pull request Nov 28, 2018
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