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

Added testling-ci integration. Fixes #124 #149

Closed
wants to merge 9 commits into from

Conversation

twolfson
Copy link

In this PR, I have adjusted the package.json and test/browser/index.html to use a common set of JS files such that we can hook into testling-ci.

The advantage of using testling-ci is that it can test against a large number of browsers and presents you with a badge for each commit.

There will be one additional step taken before merging this PR; add the testling-ci GitHub hook to chaijs/chai.

The badge for my forks looks like:
testling-ci results
https://ci.testling.com/twolfson/chai

@@ -702,7 +702,7 @@ suite('expect', function () {

err(function(){
expect(2).to.satisfy(matcher, 'blah');
}, "blah: expected 2 to satisfy [Function: matcher]");
}, /blah: expected 2 to satisfy \[Function: matcher\s*\]/);
Copy link
Author

Choose a reason for hiding this comment

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

This was a patch so that IE10 would pass its test. It turns out named functions in IE10 look like "[Function: matcher ]" vs normal browsers with "[Function: matcher]".

This can be seen on https://ci.testling.com/twolfson/chai under d753a4a -> IE10 -> not ok 80 expect satisfy

Copy link
Member

Choose a reason for hiding this comment

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

Can you include in a different PR? Would like to get it included asap (ci stuff might take longer) and want to make sure you get credit for the contribution.

Copy link
Author

Choose a reason for hiding this comment

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

Sure thing. I was hesitant on tossing this in here but it seemed like such a small change. One updated PR coming up!

@thanpolas
Copy link

💯 chai must pass all tests so we can know what's happening in our apps.

I am currently blocked from ci because chai breaks on ie < 10

@twolfson
Copy link
Author

@thanpolas Could you please clarify on your comment? I don't understand what you are requesting. Do you want me to pull out all failing browsers from the testling-ci results?

@thanpolas
Copy link

I want chai to pass all broweser tests and testling-ci is a good start!

On Saturday, April 13, 2013, Todd Wolfson wrote:

@thanpolas https://github.com/thanpolas Could you please clarify on
your comment? I don't understand what you are requesting. Do you want me to
pull out all failing browsers from the testling-ci results?


Reply to this email directly or view it on GitHubhttps://github.com//pull/149#issuecomment-16330290
.


Thanasis Polychronakis

http://thanpol.as/http://thanpol.as/?utm_source=email&utm_medium=email&utm_campaign=email

@twolfson
Copy link
Author

If you want all browsers to pass, then that is not possible. IE6/7 cannot create the same fluent interface for expect and should. i.e.

expect(apple).to.have.property('color', 'red');

This is since they both lack __defineGetter__ and defineProperty.

However, IE8 might be possible, IE9 just needs tweaks to get it working, and I have no idea what is going on with Opera 11.5. I guess I will pull them from the package.json for now as the purpose of this PR was to fix #124, "Document Supported Browsers", and nothing more.

Those are separate issues and should be addressed in their own PR's.

@thanpolas thanpolas mentioned this pull request Apr 14, 2013
@logicalparadox
Copy link
Member

First of all I wanted to thank you (@twolfson) for bringing up the issue of browser ci and the work you have put in to trying to get it running. Chai does need automated visibility into what browsers are supported... I just don't think testling-ci is the answer to this.

For example, the fail for IE9 is misleading. Chai support IE9 for the assert and expect interfaces but we know it is not possible to get it running for the should interface.

In the last major release I noted that preliminary support had been added for sauce labs open source. The general idea being that each interface's tests could be run in the browsers which chai officially supports. The documentation would then include a grid of browser to interface ci status to satisfy the need for transparency.

Overall it is probably a little more difficult to setup but I think it results in a more complete solution for the long term. Unfortunately I have not had the chance to finish this yet, but if you are interested in giving it a shot:

  • You will need an [open source account] from sauce labs. (its free)
  • Uses mocha-cloud to connect mocha with sauce labs.
  • You can view what has been already started in ./support/mocha-cloud.js.

Check it out and let me know what you think.

@twolfson
Copy link
Author

I understand and agree with most of your points. I saw the test/browser/sauce.html and figured that was something relevant.

testling-ci is still pretty young as well and has a long way to go. It lacks per-branch testing and per-browser test cases (although that is an edge case itself).

As far as the current badge goes, did you want me to pull out the failing browsers or leave them in there as an indicator? I know @thanpolas and I had mixed opinions on it.

@twolfson
Copy link
Author

Updated PR to not include IE10 patch but having troubles with testling-ci updating. Asking about it in irc.

@logicalparadox
Copy link
Member

Yes, it is quite an edge case. I would rather not include a badge that can be misleading in any way (especially since it is already in debate). If testling-ci cannot accomplish these goals then including it seems to be a moot action as it will just be removed later.

@twolfson
Copy link
Author

Agreed -- maybe we should add a notice next to the badge about that for the interim (e.g. "This represents browsers that support the full functionality. In less modern browsers, assert and expect will work.)? Or just close the PR altogether?

@twolfson
Copy link
Author

@logicalparadox Alright, testling-ci is back to being functional. The latest badge looks like this:

testling-ci
https://ci.testling.com/twolfson/chai

I would suggest adding testling-ci for the interim since the journey to Selenium/Sauce Labs will not be an immediate victory and the work for this is already complete.

If you want me to change anything else (e.g. add wording, add/remove browser), please let me know. Thanks!

@thanpolas
Copy link

For example, the fail for IE9 is misleading. Chai support IE9 for the assert and expect interfaces but we know it is not possible to get it running for the should interface.

This is not true. IE9 fails using the assert interface. Closing the issue will not make it go away #150

@balupton
Copy link

Would really love testling and full fledged browser support for Chai. We use chai for all our node.js probjects, and with testling we can test the browser compatible ones as well.

One question though, is how does testling know what tests fail? Doesn't testling depend on TAP output? Couldn't find any support for TAP within Chai.

@twolfson
Copy link
Author

Chai's should/expect syntax will not work in all browsers as it depends on ES5 getters/setters.

The tests themselves are written with the mocha framework which has a TAP reporter.

Chai is an assertion library and does not know anything about the state of tests. In an over-simplification, it throws semantic errors to be interpreted by a testing framework (if present).

@balupton
Copy link

Chai's should/expect syntax will not work in all browsers as it depends on ES5 getters/setters.

Ah okay, so that is how expect(blah).to.be.null works without a function call at the end! I always thought that it was just returning class instances... My bad.

The tests themselves are written with the mocha framework which has a TAP reporter.

Chai is an assertion library and does not know anything about the state of tests. In an over-simplification, it throws semantic errors to be interpreted by a testing framework (if present).

Ah gotcha, so this pull request is just about getting the test suite for chai working in testling, not chai itself. So for myself, I'd just have to make sure whatever test runner I use supports tap. Got it.


So all in all, if we want IE support, we got to look elsewhere, or do something radical like rewriting chai to use function calls instead of getters/setters, so expect(blah).to().be().null()

@twolfson
Copy link
Author

Yep. Another suggestion I had was to support strings as if they were assessors of properties (#154). It would require a lot less work from developers and have a similarly clean syntax (where as the invocations get dirty).

@logicalparadox
Copy link
Member

Going different direction. See #195

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.

4 participants