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

[FEATURE] ember-testing-checkbox-helpers #9798

Merged
merged 1 commit into from Jan 5, 2015

Conversation

Projects
None yet
5 participants
@seanpdoyle
Contributor

seanpdoyle commented Dec 3, 2014

The fillIn test helper should be able to handle checking and unchecking an
<input type="checkbox">. The helper can infer the input type based on the
provided value. If the value is a boolean, the input is assumed to be a
checkbox. If not, the input is assumed to support text.

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:fill-in-checkboxes branch Dec 3, 2014

@seanpdoyle

This comment has been minimized.

Contributor

seanpdoyle commented Dec 3, 2014

Should this be behind a feature flag? It doesn't change the API, it makes it more robust.

@rwjblue

This comment has been minimized.

Member

rwjblue commented Dec 3, 2014

Why use fillIn for a checkbox? I would generally use click I think....

@seanpdoyle

This comment has been minimized.

Contributor

seanpdoyle commented Dec 3, 2014

@rwjblue there is no way to determine at click-time whether or not the box is already checked, the value is toggled no matter what.

This allows ensuring that a checkbox has the declared check state, no matter the precondition.

@stefanpenner

This comment has been minimized.

Member

stefanpenner commented Dec 5, 2014

going to briefly talk about this at todays meeting.

can you rebase?

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:fill-in-checkboxes branch Dec 5, 2014

@seanpdoyle

This comment has been minimized.

Contributor

seanpdoyle commented Dec 5, 2014

@stefanpenner 👍 rebased and still passing

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:fill-in-checkboxes branch 2 times, most recently Dec 11, 2014

@mixonic

This comment has been minimized.

Member

mixonic commented Dec 13, 2014

I think this slipped past at the meeting @seanpdoyle. I've tagged it for next week though. Sorry for the delay.

@rwjblue

This comment has been minimized.

Member

rwjblue commented Dec 21, 2014

We discussed in the meeting yesterday, and we would prefer to create a new helper instead of changing fillIn (primarily because fillIn as a word doesn't really make much sense in the context of a checkbox). We would prefer to add check and uncheck helper methods.

@seanpdoyle - Can you update this PR adding those helpers and a feature flag for it (ember-testing-checkbox-helpers)?

@seanpdoyle

This comment has been minimized.

Contributor

seanpdoyle commented Dec 21, 2014

@rwjblue I agree, that's a good idea.

Should the helper blowup if the selector doesn't match a checkbox?

How do the other helpers behave?

@rwjblue

This comment has been minimized.

Member

rwjblue commented Dec 21, 2014

Yes, it should use the relatively private findWithAssert to find the checkbox, and verify that the element returned is a checkbox (if not have a nice error/assertion that indicates that the check helper only supports checkbox type inputs).

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:fill-in-checkboxes branch Dec 22, 2014

@seanpdoyle

View changes

packages/ember-testing/tests/helpers_test.js Outdated
expectAssertion(function() {
check('#text');
}, /must be a checkbox/);

This comment has been minimized.

@seanpdoyle

seanpdoyle Dec 22, 2014

Contributor

@rwjblue these tests are failing, both complaining that Ember.assert is never run, and raising an exception from the assertion.

Is there something I'm missing when expecting an assertion from an async helper?

screen shot 2014-12-22 at 11 44 51 am

This comment has been minimized.

@seanpdoyle

This comment has been minimized.

@rwjblue

rwjblue Dec 22, 2014

Member

Try:

expectAssertion(function() {
    run(function() {
      check('#text');
    });
});

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:fill-in-checkboxes branch Dec 22, 2014

@seanpdoyle seanpdoyle changed the title from [BUG] `fillIn` test helper should support checkbox to [FEATURE] ember-testing-checkbox-helpers Dec 22, 2014

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:fill-in-checkboxes branch Dec 22, 2014

@rwjblue

View changes

packages/ember-testing/tests/helpers_test.js Outdated
visit('/');
expectAssertion(function() {
check('#text');

This comment has been minimized.

@rwjblue

rwjblue Dec 22, 2014

Member

Need to do this inside of a run I believe.

This comment has been minimized.

@seanpdoyle

seanpdoyle Dec 22, 2014

Contributor

@rwjblue the check call or the expectAssertion call?

I've wrapped the check and the expectAssertion calls to no avail

@seanpdoyle

This comment has been minimized.

Contributor

seanpdoyle commented Dec 22, 2014

@rwjblue wrapping the check in a run block doesn't seem to do the trick. If this is a blocker, I could remove the tests to get things green and leave the Ember.assert in there.

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:fill-in-checkboxes branch 3 times, most recently Jan 2, 2015

visit('/').then(function() {
expectAssertion(function() {
check('#text');
}, /must be a checkbox/);

This comment has been minimized.

@seanpdoyle

seanpdoyle Jan 2, 2015

Contributor

@rwjblue this seems to have done the trick.

I think this is ready for another look.

@rwjblue

This comment has been minimized.

Member

rwjblue commented Jan 2, 2015

Can you squash the commits into one that is prefixed with [FEATURE ember-testing-checkbox-helpers]?

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:fill-in-checkboxes branch Jan 2, 2015

@seanpdoyle

This comment has been minimized.

Contributor

seanpdoyle commented Jan 2, 2015

@rwjblue squashed.

@rwjblue

This comment has been minimized.

Member

rwjblue commented Jan 2, 2015

Phantom crashed during the Travis run, I restarted it.

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:fill-in-checkboxes branch Jan 2, 2015

@seanpdoyle

This comment has been minimized.

Contributor

seanpdoyle commented Jan 2, 2015

@rwjblue https://travis-ci.org/emberjs/ember.js/jobs/45707207#L651 seems to be the problem. I'm not sure why it's crashing.

screen shot 2015-01-02 at 2 02 22 pm

Works on my machine ¯_(ツ)_/¯

[FEATURE ember-testing-checkbox-helpers]
Introduce the `check` and `uncheck` test helpers.

Add `check` and `uncheck` test helpers.

`check`:

Checks a checkbox. Ensures the presence of the `checked` attribute

Example:

```javascript
check('#remember-me').then(function() {
  // assert something
});
```

`uncheck`:

Unchecks a checkbox. Ensures the absence of the `checked` attribute

Example:

```javascript
uncheck('#remember-me').then(function() {
  // assert something
});
```

@seanpdoyle seanpdoyle force-pushed the seanpdoyle:fill-in-checkboxes branch to 92db448 Jan 5, 2015

@seanpdoyle

This comment has been minimized.

Contributor

seanpdoyle commented Jan 5, 2015

@rwjblue after a rebase this is now passing in CI

rwjblue added a commit that referenced this pull request Jan 5, 2015

Merge pull request #9798 from seanpdoyle/fill-in-checkboxes
[FEATURE] ember-testing-checkbox-helpers

@rwjblue rwjblue merged commit e6b0369 into emberjs:master Jan 5, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@rwjblue

This comment has been minimized.

Member

rwjblue commented Jan 5, 2015

Awesome, thank you!

@seanpdoyle seanpdoyle deleted the seanpdoyle:fill-in-checkboxes branch Jan 5, 2015

@tomdale

This comment has been minimized.

Member

tomdale commented Jan 9, 2015

After reviewing this, we like the idea but have some concerns about both the implementation and semantics. @rwjblue will file an issue with more details, but at a high level:

  1. Can we enumerate what use cases this is intended for? It's unclear why this is better than click(), other than it's slightly more expressive. The use cases should drive the semantics, which I think are incorrect right now. It's unclear to me why tests are ever so non-deterministic that you don't know whether a click turns it on or off in that particular test.
  2. Right now this always triggers a change event, even if the click would be a no-op (if the checkbox was already checked, for example, or if the checkbox is disabled). I am extremely nervous about introducing test helpers that diverge from behavior that actual users can perform.

seanpdoyle added a commit to seanpdoyle/ember.js that referenced this pull request Jan 26, 2015

[FEATURE] ember-testing-checkbox-helpers
fixes emberjs#9798 (emberjs#9798 (comment))

When test driving, `click` and `unclick` are useful for ensuring the
state of a form.

```javascript
export default DS.Model.extend({
  title: DS.attr(),
  body: DS.attr(),
  isDraft: DS.attr("boolean", defaultValue: false)
});

test("doesn't publish posts in draft mode", function() {
  fillIn("#title", "this post is hidden");
  fillIn("#body", "this won't be displayed");
  check("#is-draft");
  click("#submit");

  andThen(function() {
    equal(find(".post:contains('this post is hidden')").length, 0);
  });
});
```

`check` and `uncheck` allow the test to remain unchanged if the
`defaultValue` of `isDraft` changes from `false` to `true`.

The test only cares that:

* when `click("#submit")` executes, the `#is-draft` checkbox is `checked`
* `isDraft` is true

seanpdoyle added a commit to thoughtbot/ralphs-little-helpers that referenced this pull request Nov 1, 2015

Introduce `check` and `uncheck` async helpers
Promotes `check` and `uncheck` test helpers from [emberjs/ember.js] from
behind the [ember-testing-checkbox-helpers] feature flag.

Matches Capybara's [#check] and [#uncheck] helper methods.

Additionally introduces `assert.checked` and `assert.unchecked` QUnit
assertions.

[ember-testing-checkbox-helpers]: https://github.com/emberjs/ember.js/blob/860bb4be498466ac4c914adb1f139e5cd2fbc123/FEATURES.md#feature-flags
[emberjs/ember.js]: emberjs/ember.js#9798
[#check]: http://www.rubydoc.info/github/jnicklas/capybara/master/Capybara/Node/Actions#check-instance_method
[#uncheck]: http://www.rubydoc.info/github/jnicklas/capybara/master/Capybara/Node/Actions#uncheck-instance_method

seanpdoyle added a commit to thoughtbot/ralphs-little-helpers that referenced this pull request Nov 1, 2015

Introduce `check` and `uncheck` async helpers
Promotes `check` and `uncheck` test helpers from [emberjs/ember.js] from
behind the [ember-testing-checkbox-helpers] feature flag.

Matches Capybara's [#check] and [#uncheck] helper methods.

Additionally introduces `assert.checked` and `assert.unchecked` QUnit
assertions.

[ember-testing-checkbox-helpers]: https://github.com/emberjs/ember.js/blob/860bb4be498466ac4c914adb1f139e5cd2fbc123/FEATURES.md#feature-flags
[emberjs/ember.js]: emberjs/ember.js#9798
[#check]: http://www.rubydoc.info/github/jnicklas/capybara/master/Capybara/Node/Actions#check-instance_method
[#uncheck]: http://www.rubydoc.info/github/jnicklas/capybara/master/Capybara/Node/Actions#uncheck-instance_method

seanpdoyle added a commit to thoughtbot/ralphs-little-helpers that referenced this pull request Nov 1, 2015

Introduce `check` and `uncheck` async helpers
Promotes `check` and `uncheck` test helpers from [emberjs/ember.js] from
behind the [ember-testing-checkbox-helpers] feature flag.

Matches Capybara's [#check] and [#uncheck] helper methods.

Additionally introduces `assert.checked` and `assert.unchecked` QUnit
assertions.

[ember-testing-checkbox-helpers]: https://github.com/emberjs/ember.js/blob/860bb4be498466ac4c914adb1f139e5cd2fbc123/FEATURES.md#feature-flags
[emberjs/ember.js]: emberjs/ember.js#9798
[#check]: http://www.rubydoc.info/github/jnicklas/capybara/master/Capybara/Node/Actions#check-instance_method
[#uncheck]: http://www.rubydoc.info/github/jnicklas/capybara/master/Capybara/Node/Actions#uncheck-instance_method

seanpdoyle added a commit to thoughtbot/ralphs-little-helpers that referenced this pull request Nov 3, 2015

Introduce `check` and `uncheck` async helpers
Promotes `check` and `uncheck` test helpers from [emberjs/ember.js] from
behind the [ember-testing-checkbox-helpers] feature flag.

Matches Capybara's [#check] and [#uncheck] helper methods.

Additionally introduces `assert.checked` and `assert.unchecked` QUnit
assertions.

[ember-testing-checkbox-helpers]: https://github.com/emberjs/ember.js/blob/860bb4be498466ac4c914adb1f139e5cd2fbc123/FEATURES.md#feature-flags
[emberjs/ember.js]: emberjs/ember.js#9798
[#check]: http://www.rubydoc.info/github/jnicklas/capybara/master/Capybara/Node/Actions#check-instance_method
[#uncheck]: http://www.rubydoc.info/github/jnicklas/capybara/master/Capybara/Node/Actions#uncheck-instance_method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment