Spec update

[timwienk]

I decided to update our specs a bit, for a number of reasons:

1. I noticed that we use two incompatible versions of Jasmine within Core (1.3 and 2.3),
2. The server specs seemed a bit incomplete,
3. Mocha seems to be the most popular one nowadays, so for interoperability (say we want to include third party specs, or someone else wants to include Moo specs) this seems the most flexible.

When I started doing this, I didn't know how much work it'd be, and luckily it wasn't too bad. The problem is, though, that it'll look like a lot and it'll be a hell to review because of the sheer number of changes.

To be able to review it I split it up in parts, and I would like to suggest to review it by part, we could even merge it in by part. I didn't create Pull Requests, because every next Pull Request would include the changes from the previous Pull Request since they're not merged at this point. When reviewed, I guess they could be merged in, and I could actually create a Pull Request for every next one to make that easier.

So, the parts:

• part 1: Improvements to three specs I ran into. $$ spec actually turned out to be broken after adding actual elements to select.
• part 2: A little housekeeping and updating of package.json, server tests and .travis.yml.
• part 3: First "big" change, removed Tests/Utilities and included syn and sinon packages instead, added Tests/Plugins/syn to use syn in karma. Last commit re-adds a local version of syn (hopefully temporarily).
• part 4: Update everything to Jasmine to 2.3, major changes: spy API and async API. Instead of using the new Jasmine spy API, I changed these to sinon spies. The new async API is very similar to the mocha async API.
• part 5: Changed the server specs to be able to test more than just Core/*.js files (which were actually included 5 times).
• part 6: First commit: replacement of Jasmine assertions with Chai assertions. Second commit: replacement of Chai with expect.js. Third commit: replacement of Jasmine with Mocha.

At the point of any of these commit the specs should still run fine and turn all green, I made the changes step-by-step to not accidentally change behaviours. (In fact, if we want to stick with Jasmine, we could decide to stop after part 5, or even after the expect.js commit of part 6.) The only exception to this is the Chai commit of part 6, everything runs fine, but Chai doesn't support IE older than 9, keeping the commit for history and reasoning, though.

In case it's easier (it probably is), I should be in IRC most of the time for comments or anything related.

Note for later: There are some things I didn't touch, like the test "bootstrapping". I think after these changes, it might be a good idea to look at the Gruntfile.js and files in the Tests directory. It looks like these things could be restructured to be easier to understand.

[SergioCrisostomo]

I read thru the changes and found nothing strange/wrong. Sent it to Travis also. Got green all over. I'm easy about changing to Mocha or staying with Jasmine.

Nice job! Thank you for the time you put into this! :+1:

[arian]

Very nice!

A few things:

• Some tests are disabled with using xdescribe, for example https://github.com/timwienk/mootools-core/compare/spec-update-part-3...spec-update-part-4#diff-d7242700ac47efe4e538390355c32968R50

  Is that still valid, or was it disabled at some point and never enabled again?

• chai uses expect(..).to.be.true, where true is a keyword. iirc that doesn't work in IE8, and should be to.be['true']. Is that relevant? (https://github.com/chaijs/chai/issues/124#issuecomment-11639702)

[timwienk]

Some tests are disabled with using xdescribe, for example https://github.com/timwienk/mootools-core/compare/spec-update-part-3...spec-update-part-4#diff-d7242700ac47efe4e538390355c32968R50 Is that still valid, or was it disabled at some point and never enabled again?

The specs where there's now xdescribe, there used to be a xit, so yea, they were disabled and never re-enabled. I believe they're tests from our old spec engine with PHP stuff. [intended]

I did just realise that if they were enabled, they'd fail now, because Mocha uses arguments to done as reasons a spec failed, while Jasmine has done.fail for that. [fixed]

chai uses expect(..).to.be.true, where true is a keyword. iirc that doesn't work in IE8, and should be to.be['true']. Is that relevant? (https://github.com/chaijs/chai/issues/124#issuecomment-11639702)

That's a good point. I kind of assumed they made it work. The best workaround is probably to use replace to.be.true with to.equal(true). The same thing should be the case for to.be.false and to.be.null. Will look into that. [fixed]

It should be fine for to.be.undefined, however, for the sake of consistency, maybe we should replace those as well. [fixed]

[arian]

That's a good point. I kind of assumed they made it work. The best workaround is probably to use replace to.be.true with to.equal(true). The same thing should be the case for to.be.false and to.be.null. Will look into that.

This is one of the reasons I initially used https://github.com/Automattic/expect.js (and kept using it since, I think chai wasn't really a thing then, the other choice was 'should.js').

[timwienk]

This is one of the reasons I initially used https://github.com/Automattic/expect.js (and kept using it since, I think chai wasn't really a thing then, the other choice was 'should.js').

Chai seems a very complete solution, including assert style assertions as well, which could benefit us in the future. However, if things don't work in IE older than 9, we should probably switch. Although I prefer some specific things Chai did with the expect syntax, the syntax of all expect style things are very similar anyway. [fixed]

[timwienk]

After looking at things, they clearly state they don't support IE<9, so Chai is not an option. I replaced it with the library Arian suggested, expect.js. [fixed]

[arian]

:+1:

[timwienk]

New issues encountered:

• Specs fail in IE7, because Karma v0.13 does not work in IE7. [fixed]
  • Reason: Karma v0.13 uses a newer version of socket.io that causes problems in IE7. Supposedly this is fixable by passing transports: ['polling'], forceJSONP: true to socket.io, but Karma doesn't allow for the forceJSONP option to be passed right now.
  • Solution: downgrade back to v0.12.
• Specs fail in IE>10, because Sinon v1.17 does not work in IE>10. [fixed]
  • Reason: Things break in a very weird way when doing the IE-compat stuff from within a conditional on IE11 (and Edge).
  • Solution: downgrade to v1.14, until sinonjs/sinon#848 is fixed.
  • Alternative solution (in case it doesn't get fixed): we could change our environment to only load sinon-ie for old IE, this would be annoying when testing locally(/manually), since you need different karma-plugins for different envs.
• Specs fail in IE9 and IE10, because no current version of syn works in IE9 and IE10. [fixed]
  • Reason: The feature test for <a href="javascript:something()" /> in IE9 and IE10 causes a page reload of which Karma doesn't approve.
  • Solution: use a local version that does work (hopefully temporarily). I've described the issue in bitovi/syn#105.

Latest test results: https://travis-ci.org/timwienk/mootools-core/builds/82323286

[SergioCrisostomo]

Nice work!

I am ok with merging these to master branch.

[timwienk]

@arian: Add node_js 4 here as well?

Never mind, it's already included in the other matrix.

Yes, what I changed in .travis.yml is: instead of adding the multiple node_js versions when building the matrix, only one is used for that, and the non-current versions are "manually" included after the matrix is built for only server testing. That way we don't have 75 runs of which 48 actually do nothing.

@arian: However if you use version 4, will it automatically use the latest 4.x version? As they are using proper semver now, all 4.x versions should automatically work.

That's a good point, I'll change that and hopefully they indeed don't break within the major version. :-D [fixed]

https://travis-ci.org/timwienk/mootools-core/builds/82406894

[timwienk]

Okay, so everything seems to work. Now for the last two questions:

1. Do we agree on switching to Mocha? What are the reasons not to?
2. When we go and merge, shall I PR this:
   • with everything in one PR,
   • as 6 separate PRs; one per part,
   • or two PRs; one for the first 5 parts, one for the mocha stuff?

[arian]

1. Because of the effort of porting it. But you already did this.
2. Two PRs would be a nice separation I think.

[SergioCrisostomo]

I say same as @arian.