AmpersandJS / ampersand-sync

Provides sync behavior for updating data from ampersand models and collections to the server.
http://ampersandjs.com
MIT License
20 stars 28 forks source link

update testing framework #75

Closed wraithgar closed 8 years ago

wraithgar commented 8 years ago

Switch fully to zuul Add saucelabs config Use node LTS and npm 2 for building

See discussion that led to this here: AmpersandJS/ampersand-filtered-subcollection#26

naugtur commented 8 years ago

This is very cool. I wanted to go ~this way with the tests eventually. Some questions:

  1. Is it possible to replace phantom with electron? From my recent experience it's so much better at being a browser :) I ll check if zuul supports it.
  2. Things are timing out, looks like the infrastructure that runs it is not permitting outbound request (which sync tests use)
  3. Would you mind if I used your configuration in my other projects?
wraithgar commented 8 years ago

I think phantom is fine for local tests. I don't think it matters as much as the CI tests which are being run against real browsers.

Yeah timing out was a huge travis-ci problem yesterday. Hopefully it's better today. Quite bad timing that travis was having all those problems on the day I decided to tackle this.

This code is, as always, MIT licensed ;)

wraithgar commented 8 years ago

Looks like we're relying on mocky.io for these tests and that's what's timing out, but only from saucelabs.

wraithgar commented 8 years ago

Gonna get the other tests working first then come back and refactor the integration tests to run something like sinonjs. http://sinonjs.org/docs/#server

naugtur commented 8 years ago

I made these tests send real requests so that they'd cover XDomain issues in older browsers too. They used to stub the whole xhr module and they failed to notice change in it. That's why the rewrite happened.

As long as you stub the XMLHttpRequest, it should be fine. Pro tip: xhr provides a way to inject the stub (because it caches a reference to XMLHttpRequest internally and you'd have to require it after stubbing, which would suck)

Not a lot of free time recently, but I could help if you're not in a rush.

wraithgar commented 8 years ago

Yeah as far as I can tell that's exactly what sinonjs does (stub the xmlhttprequest). Gonna try to get it working this afternoon, thanks for the heads up on xhr.

naugtur commented 8 years ago

I've been using sinon quite a while and that's what I've been implying here :)

It's a bit tricky, because at first everyone tends to try using the stub handle while it's the global XMLHttpRequest that is replaced.

This should work:

var xhr = require("xhr")
var stubHandle = sinon.useFakeXMLHttpRequest()
xhr.XMLHttpRequest = XMLHttpRequest; //becasue it had the original one
wraithgar commented 8 years ago

Wow you're a saint, thank you so much for solving the problem already. I'm doing this one last, will start on it after I get the tests in ampersand-view working.

^5

wraithgar commented 8 years ago

What a nightmare. Turns out zuul uses xhr and also makes xmlhttprequests during the test suite. Spent all day tracking that little quirk down.

These tests will not work anymore under node, as xhr returns the request library under those circumstances.

naugtur commented 8 years ago

Ouch. Looks like you had fun debugging.

I'm on my phone and I only had a moment to look at the code, but I don't see why not just go with jquery.noconflict algorithm here.

var xhr = require("xhr")
var stubHandle = sinon.useFakeXMLHttpRequest()
var a=xhr.XMLHttpRequest
xhr.XMLHttpRequest = XMLHttpRequest
XMLHttpRequest=a;
naugtur commented 8 years ago

I was thinking about it in the background today and I have an idea for a tool that'd solve this without regressing on node tests.

wraithgar commented 8 years ago

The noconflict approach wouldn't make any difference cause zuul is requiring xhr too, haha.

wraithgar commented 8 years ago

I think this one's ready to go @AmpersandJS/core-team it still maintains IE9+ support.

lukekarrys commented 8 years ago

+1

wraithgar commented 8 years ago

@naugtur would you be cool w/ merging this till you can make your ideas into another PR?

naugtur commented 8 years ago

Sure. It doesn't affect functionality and tests on browsers are a priority. +1

Also, thanks for asking. :)