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

Revamping testing suit. #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Revamping testing suit. #45

wants to merge 1 commit into from

Conversation

dmill-bz
Copy link
Collaborator

@dmill-bz dmill-bz commented Apr 7, 2016

This is a replacement for PR #37.
You will notice that the coverage goes down significantly and this is normal. I'll go into detail about it later.

Short version

  • Some minor cleaning
  • Added karma and browser testing support.
  • Removed coverage from node tests and placed them into browser tests. See (**) for more information. But now if you check coveralls you will see that each line is accounted for and we can see what hasn't been tested.
  • Travis tests both node and browser now

The problem

I finally figured out why things weren't working here. It's because the current tests are run against lib and we want to map them to src. The solution is to run the tests against src directly. I haven't really looked into how that would go for the mocha + node combination. But since I've figured it out for karma I decided to run coverage in karma instead (shouldn't be any drawbacks).

Why coverage is so low

Because karma is doing the coverage against src and all the current tests are done against lib the coverage ignores those tests. I've added a GremlinClientTest.js demonstrating a test that does proper coverage (you can see this in coveralls).

We have two options here. We either adapt the current tests to use src (including mocha+node configuration). Or we add a new set of (slightly redundant) tests just like GremlinClientTest

Long Version

  • Added all the required dependencies for karma.

  • Updated babel and some other dependencies.

  • Removed Gulp references (I think we don't care for this anymore?)

  • Same for browserify

  • Added Karma configuration. this file is self explanatory for the most part (should be properly commented, but let me know if something looks weird. Bellow is some of the less clear stuff:

    We load a tests.webpack.js into karma and karma runs all it's tests agains this file. This file goes into the test folder and retrieves all *Test.js files. It runs these tests (hence the renaming of files).

    I've provided a few npm run test:* commands, the difference between test:browser and test:browser:debug is that the debug leaves the browser open. The error messages there make often more sense than the ones in the terminal.

    Firefox is required. Phantom.js has a few issues that are a pain to deal with. Besides Firefox is also supported by travis.

  • Added a test, we can remove this depending on the direct we take to improve coverage

I think that's it

@coveralls
Copy link

Coverage Status

Coverage decreased (-46.03%) to 45.695% when pulling 4c19f33 on testing-revamp into 4583662 on master.

'tests.webpack.js': [ 'webpack', 'sourcemap'] //preprocess with webpack and our sourcemap loader
},

//~ babelPreprocessor: {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could clean this

@jbmusso
Copy link
Owner

jbmusso commented Apr 13, 2016

Ah, this is such a nice addition. I'll take some time this weekend to review it.

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

3 participants