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

Add Grunt-powered Jasmine spec runner and a first spec #1462

Merged
merged 4 commits into from
Mar 28, 2015

Conversation

leonardehrenfried
Copy link
Contributor

Hi Chosen,

as mentioned in #1401 there seems to be some appetite for introducing testing to chosen.

This pull request does exactly that by using the great jasmine spec runner for Grunt. It installs phantomjs through npm and uses that to run the tests. If you're not familiar with phantomjs: it's a headless webkit browser that is as good as a real browser.

The specs themselves are fairly simple so far but I tried to show an example of constructing a chosen element entirely in memory, doing some interactions on it and asserting the expected result.

If you want to run the tests just do a

npm install
grunt test

If you have any questions, please shoot. I'd love to incorporate your feedback into this PR.

@@ -92,6 +93,13 @@ module.exports = (grunt) ->
src: ['public/**/*']
dest: "chosen_#{version_tag()}.zip"

jasmine:
jquery:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to have tests for the prototype version as well (and sharing things as much as possible to ensure they behave the same)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems a little tricky but definitely doable. Maybe factoring out the part that creates the actual chosen from the part that interacts with it and does the asserts would be possible.

Is the lack of Prototype tests something that would stop this PR being from being merged?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, it would be better if it could be added. But having tests only for the jQuery version is already better than no tests at all

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Lenniboy do you have an idea of how we could share specs between the jQuery and Prototype versions as much as possible ? Applying chosen will need to be different in both cases for instance.

Maybe we could load a small test helper with 2 different implementation. One method could be apply_chosen(element, options = {}, which would apply chosen according either the jqury way or the prototype way.
However, the tests themselves are currently relying on jQuery, and I would like to avoid it when testing Prototype (a common mistake done in recent PRs is to use jQuery in the common code where it should not be used). Do you have an idea for this ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@stof
Copy link
Collaborator

stof commented Sep 3, 2013

@Lenniboy what is the status on this PR. Is the current state working or is there some missing stuff to have a working test architecture ?

Another thing that should be added in this PR is the config file for Travis, so that tests are launched automatically on each push and on each PR.

@harvesthq/chosen-developers should we create a branch in the official repo for this already to finish the work or should it stay in the fork for now ?

@leonardehrenfried
Copy link
Contributor Author

I am happy for this to merged. Yes, the test suite isn't covering a lot but
I intended it as a start as the core developers indicated that they would
like chosen development to become more test driven. I'm happy to answer
questions regarding TDD and JS.

I can add the Travis integration in a separate pull request.

On Wednesday, 4 September 2013, Christophe Coevoet wrote:

@Lenniboy https://github.com/lenniboy what is the status on this PR. Is
the current state working or is there some missing stuff to have a working
test architecture ?

Another thing that should be added in this PR is the config file for
Travis, so that tests are launched automatically on each push and on each
PR.

@harvesthq/chosen-developers should we create a branch in the official
repo for this already to finish the work or should it stay in the fork for
now ?


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

@tjschuck tjschuck mentioned this pull request Jan 26, 2015
@adunkman adunkman mentioned this pull request Mar 28, 2015
@pfiller pfiller merged commit a5723f5 into harvesthq:master Mar 28, 2015
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