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 #1393

Closed
wants to merge 16 commits into from
Closed

Karma support #1393

wants to merge 16 commits into from

Conversation

kum-deepak
Copy link
Collaborator

@kum-deepak kum-deepak commented Mar 31, 2018

Initial support for Karma. It works!

After running npm i run ./node_modules/.bin/karma start. Current configuration will run all test cases using Chrome and Firefox.

Next steps:

@kum-deepak kum-deepak changed the base branch from develop to 3.0 March 31, 2018 03:06
@kum-deepak
Copy link
Collaborator Author

kum-deepak commented Mar 31, 2018

@kum-deepak
Copy link
Collaborator Author

Figured out why sauce labs tests fail for pull requests, from: https://docs.travis-ci.com/user/environment-variables/#Defining-encrypted-variables-in-.travis.yml

Encrypted environment variables are not available to pull requests from forks due to the security risk of exposing such information to unknown code.

test sauce labs
@kum-deepak
Copy link
Collaborator Author

Sauce labs tests are working, for testing I have used my credentials for that service. Please see:

https://travis-ci.org/kum-deepak/dc.js/builds/360637386

@gordonwoodhull
Copy link
Contributor

Fantastic!

@kum-deepak
Copy link
Collaborator Author

kum-deepak commented Mar 31, 2018

I think I have tested everything I had planned - all are working. Next decision points:

  • I have created 3 sets of configuration for Karma, one each for unit (local tests), ci (Travis full test) and ci-pull (Travis pull). We need to finalize configuration for each of these setups.
  • Set of browsers
  • Concurrency
  • Reporters - dots, summary, saucelabs (can be a combination)
  • Log level for browser console output (currently I have put it to error)
  • We need to cleanup grunt file for actions that are no longer needed

@kum-deepak
Copy link
Collaborator Author

kum-deepak commented Apr 1, 2018

  • Cleaned up package.json and Gruntfile.
  • Implemented coverage using karma-coverage, internally uses Istanbul (same as previous).
  • Added a safe wrapper for Sauce Lab tests, now if SAUCE_USERNAME/SAUCE_ACCESS_KEY is not available in process.env, it will skip instead of failing.

Not sure what browserify and the associated jasmine tests were supposed to do, once I know, will implement.

@kum-deepak
Copy link
Collaborator Author

Also enabled cache for node_modules in Travis, it might improve test timings (but it might actually make it worse).

We can also try npm ci (https://medium.com/@tomastrajan/how-to-speed-up-continuous-integration-build-with-new-npm-ci-and-package-lock-json-7647f91751a) in few months.

@gordonwoodhull
Copy link
Contributor

Merged in 3.0 alpha 4. Thanks @kum-deepak!

As for the browserify test, that just generates a browserify bundle and runs tests using that instead of regular JS dependencies. Once or twice we did something that broke browserify, but it was only when messing with the AMD support so it xoesn't happen often.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Apr 9, 2018

Hmm, got one odd error on one of two Travis runs. All tests succeeded but Chrome reported:

Chromium 62.0.3202 (Ubuntu 0.0.0): Executed 1123 of 1123 SUCCESS (10.101 secs / 0 secs)
Chromium 62.0.3202 (Ubuntu 0.0.0) ERROR
 {
   "message": "Some of your tests did a full page reload!",
   "str": "Some of your tests did a full page reload!"
 }
Chromium 62.0.3202 (Ubuntu 0.0.0) ERROR
 {
   "message": "Some of your tests did a full page reload!",
   "str": "Some of your tests did a full page reload!"
 }
Chromium 62.0.3202 (Ubuntu 0.0.0): Executed 1123 of 1123 ERROR (10.114 secs / 0 secs)

https://travis-ci.org/dc-js/dc.js/builds/363931398?utm_source=email&utm_medium=notification

Something to keep an eye on going forward.

@kum-deepak
Copy link
Collaborator Author

I am reasonably sure that our specs do not try to do that. Initially when I had set concurrency to 2, it was more often, but still random.

The next task I am planning on build infrastructure is to launch parallel tasks at Travis using their Matrix capabilities - https://docs.travis-ci.com/user/environment-variables/#Defining-Multiple-Variables-per-Item. This will also help in testing with D3v4 and D3v5

In addition, please see what we should keep as values for the following:

  • Browser console log level
  • List of browsers for ci tests

@gordonwoodhull
Copy link
Contributor

Hmm, did you get a sense of whether this is browser-specific, e.g. only happens on Chrome?

Spurious errors are really frustrating, especially for new contributors, so if we can avoid them by only using Firefox by default, maybe that is an option. We could rely on saucelabs for the cross browser / platform stuff.

Just an idea. I guess we'll see how bad the problem is over time.

I think the current set of browsers is good. I don't think there's any point in testing Opera since it's just Chrome underneath.

We should keep the versions reasonably up to date except for IE haha. Some of those versions look pretty old by now.

I'll get back to you about the logging level. I'm not sure what level the invalid SVG attribute warnings are. Those are the really annoying logs.

@kum-deepak kum-deepak deleted the karma branch April 9, 2018 05:35
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.

2 participants