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

Set up Karma and Browserify #378

Merged

Conversation

yamikuronue
Copy link
Contributor

I don't know if you want to merge this yet, but I wanted to show it to you for review to see if you like the direction I'm going here.

What I've done so far:

  • Split server-side and client-side tests
  • Hooked up Karma to run client-side tests
  • Used the browserified dist file as the input to the client-side tests
  • Removed the import statements you were using babel for from the server-side tests
  • Set up Travis to run node 4, 5, and 6 in case I accidentally missed some other babel stuff
  • Updated the makefile to run both server and client-side tests

I'm also looking into git hooks to run browserify as a pre-commit hook, but I only have that set up locally for now. That will help keep dist up to date. I'm also still staring at Istanbul docs since I've never hooked it up for client-side tests before, but I think it looks possible now that I've split it apart. Oh, and I added an expect statement to your sanity test.

@thebigredgeek
Copy link
Contributor

Nice job. Wondering if it might be better to run browser tests with mocha? That way we can reuse tests on both platforms.

@yamikuronue
Copy link
Contributor Author

We are. Karma bootstraps the front-end test running process so it can be used in CI; it builds a page with the front-end files and mocha tests, then hits it with PhantomJS. In theory, we can also use Firefox on TravisCI, but I haven't got it set up yet.

However, I did split out the tests; in the front-end, debug is already bootstrapped as a global, while the backend uses require. I want to merge them back together, but I haven't figured out quite how yet (some conditionals will be required I think).

@yamikuronue
Copy link
Contributor Author

Combining the tests turned out to be easier than I expected :) I've added coverage to the server-side tests, and set up coveralls. So now the only problem is that there's no real tests, and my first attempt to add a real test is going awry.

@thebigredgeek Do you want to help me figure out what's wrong with this test, or do you want to merge first so we don't drift too far apart?

@thebigredgeek
Copy link
Contributor

@yamikuronue yeah let's just get the existing smoke tests running on the client as well as the server

@yamikuronue
Copy link
Contributor Author

Then we are good to go once you resolve those conflicts :)

@thebigredgeek
Copy link
Contributor

I can't resolve :(. On your branch

@yamikuronue
Copy link
Contributor Author

Oh, my bad; the button was greyed out and it said something about write access so I assumed it was permissions, but the button hover text says I have to do it from the command line. Which. Well. Here goes nothing! If I do not return from the front lines of git, it was an honor working with you ;)

@coveralls
Copy link

coveralls commented Dec 20, 2016

Coverage Status

Changes Unknown when pulling f484cfe on yamikuronue:replace-babel-with-browserify into ** on visionmedia:master**.

@AccaliaDeElementia
Copy link
Contributor

not sure why your merge attempt failed @yamikuronue, but assuming i merged the way you wanted it done you should be good now. ;-)

@yamikuronue
Copy link
Contributor Author

yamikuronue commented Dec 20, 2016

Thank you @AccaliaDeElementia !!

@thebigredgeek We should be good to merge now :)

@thebigredgeek
Copy link
Contributor

Thanks! Looks like conflicts again :(. Also, can we ditch the commit hooks?

@yamikuronue
Copy link
Contributor Author

We can, but I added them because it looked like you've had some issues with not having the dist folder get committed previously?

Gosh darn conflicts...

@AccaliaDeElementia
Copy link
Contributor

merge conflict resolved (again) :-)

@coveralls
Copy link

coveralls commented Dec 21, 2016

Coverage Status

Changes Unknown when pulling f512cf2 on yamikuronue:replace-babel-with-browserify into ** on visionmedia:master**.

@thebigredgeek
Copy link
Contributor

Thanks. LGTM! Yeah, we removed dist completely. Thanks for this!

@thebigredgeek thebigredgeek merged commit dc043cf into debug-js:master Dec 21, 2016
@yamikuronue yamikuronue deleted the replace-babel-with-browserify branch December 21, 2016 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants