Skip to content

Conversation

tmcw
Copy link
Contributor

@tmcw tmcw commented Mar 1, 2013

Replaces jasmine with Mocha, Expect.js (matchers), and Sinon (spies).

Also:

  • Fixes Mercator tests (broken in master)
  • Fixes JSHint compliance of test infrastructure
  • Splits up some tests into more granular assertions

Right now --cov reports on everything but Leaflet here - not sure what's going on there.

@tmcw
Copy link
Contributor Author

tmcw commented Mar 1, 2013

This can also include less packaged javascript if we're okay with requiring people to run npm install in order to run tests.

});

expect.Assertion.prototype.near = function(expected, delta) {
delta = 0 || 10;
Copy link
Member

Choose a reason for hiding this comment

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

0 || 10 — that's weird... Why isn't the default value 1.0E-12 like above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be made lower again - as noted in the PR, these tests were all broken before and weren't running.

@mourner
Copy link
Member

mourner commented Mar 1, 2013

I think we're OK with npm install and adding more deps in package.json. Lets figure the coverage stuff before merging though, it's quite important.

The broken tests are a surprise, any idea why they weren't running or reporting any fails?

@tmcw
Copy link
Contributor Author

tmcw commented Mar 1, 2013

The broken tests are a surprise, any idea why they weren't running or reporting any fails?

The projection tests were xdefine rather than define, and Jasmine was quietly failing.

@mourner
Copy link
Member

mourner commented Mar 1, 2013

Oops... Looks like I haven't looked at it for a while.

@mourner
Copy link
Member

mourner commented Mar 4, 2013

BTW, @tmcw is coverage behavior the same in the master branch, without the changes?

@jfirebaugh
Copy link
Member

I created a simple benchmark that exhibits the same slowdown going from jasmine -> mocha as observed here. Pretty sure this is due to the different event loop implementations they use -- mocha is limited by the latency of window.postMessage. @mourner, unless you have a lead, I will bring this to TJ's attention.

@jfirebaugh
Copy link
Member

I got a performance patch merged to mocha... and it's quite a bit faster in Chrome, but jake test didn't show much improvement. :/

@mourner
Copy link
Member

mourner commented Apr 3, 2013

Awesome work John! Any clues why it's still slow when running through Testacular/PhantomJS?

@jfirebaugh
Copy link
Member

Turns out we were using an old version of mocha bundled with testacular/karma. Replaced that with mocha 1.9.0 and now it's down to 2x slower than jasmine.

@mourner
Copy link
Member

mourner commented Apr 4, 2013

OK, lets go!

mourner added a commit that referenced this pull request Apr 4, 2013
@mourner mourner merged commit 8b9eb1b into master Apr 4, 2013
@mourner mourner deleted the mocha branch April 4, 2013 08:36
@mourner
Copy link
Member

mourner commented Apr 4, 2013

@jfirebaugh oops, it seems that test coverage doesn't work now — the reports are empty. :( Although I definitely remember it working with Mocha at some time on this pull. Tried replacing Mocha back to bundled one, no luck. Could you take a quick look?

@mourner
Copy link
Member

mourner commented Apr 4, 2013

OK, tracked it down with git bisect — coverage broke after testacuar -> karma rename commit: jfirebaugh@28daa9e

@mourner
Copy link
Member

mourner commented Apr 4, 2013

Can't figure out why it could possibly break it so reported an issue: karma-runner/karma#461

@mourner
Copy link
Member

mourner commented Apr 4, 2013

Figured it out, now everything works :)

mourner added a commit that referenced this pull request Apr 4, 2013
* master:
  fix coverage reporting in Karma #1479, close karma-runner/karma#461
  Update specs for mocha
  Manage mocha dependency with npm; update to 1.9.0
  Update mocha.js
  remove testacular master hack (as 0.6 is now stable)
  Proper default for delta
  Tighter tolerance for point-near check
  Fix mercator tests, these are broken in Leaflet master.
  Use mocha
  testacular -> karma
  remove testacular master hack (as 0.6 is now stable)
  Update deps.js
  Strage behavior of inplace github editor
  Remove trailing whitespace in fix
  Extend L.Util.template
  Popup close button bugfix
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.

3 participants