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

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

Merged
merged 1 commit into from
Jan 26, 2015

Conversation

seanpdoyle
Copy link
Contributor

Address concerns that helpers could behave differently than real users.

fixes #9798 (#9798 (comment))

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

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
Copy link
Contributor Author

@tomdale addresses #9798 (comment)

@tomdale
Copy link
Member

tomdale commented Jan 26, 2015

@seanpdoyle Seems legit, would you mind writing down somewhere the answer to the other question? I want to make sure we all have the same mental model.

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.

$el.prop('checked', true).change();
});
if (!$el.prop('checked')) {
run('click', app, selector, context);
Copy link
Member

Choose a reason for hiding this comment

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

This needs to run through app.testHelpers.click. Something like app.testHelpers.click(selector, context).

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
Copy link
Contributor Author

@tomdale I've updated the commit message and PR body to provide an example use case

rwjblue added a commit that referenced this pull request Jan 26, 2015
@rwjblue rwjblue merged commit 3d652e9 into emberjs:master Jan 26, 2015
@rwjblue
Copy link
Member

rwjblue commented Jan 26, 2015

@seanpdoyle - Thanks for updating, we'll review this feature again for go/no-go (and potentially more work) at the next core team meeting.

@seanpdoyle seanpdoyle deleted the check-uncheck-test-helper-fix branch January 26, 2015 22:22
@tomdale
Copy link
Member

tomdale commented Jan 30, 2015

This feature has ended up being surprisingly controversial at the core team meetings. ;)

After a lengthy discussion, we reached consensus that, in the example you gave (defaultValue changing from false to true), we actually want the test to fail. While making code resilient to input is important, I think there's an important exception for test helpers; we should make them as explicit and fail-fast as possible.

We did reach consensus that we would like the check() and uncheck() helpers to be a combination of setting the value and asserting it becomes a specific value. In the case of check():

  1. Before mutating the value, assert that the checkbox is unchecked.
  2. Simulate a click event on the checkbox.
  3. Assert that the checkbox has been checked (to catch, for example, the disabled case).

We believe that the above implementation strikes the best balance between explicitness and clarity, and expressiveness. What do you think?

@seanpdoyle
Copy link
Contributor Author

@tomdale I like it.

Does this mean that the if blocks are replaced by book-ended Ember.assert calls?

@rwjblue
Copy link
Member

rwjblue commented Jan 31, 2015

@seanpdoyle - Yes, assert that it is unchecked before click is called, then assert that it is checked after click is called. And the same (in reverse) for unclick.

seanpdoyle added a commit to seanpdoyle/ember.js that referenced this pull request Jan 31, 2015
emberjs#10287 (comment)

> We did reach consensus that we would like the check() and uncheck()
> helpers to be a combination of setting the value and asserting it
> becomes a specific value. In the case of check():
>
> Before mutating the value, assert that the checkbox is unchecked.
> Simulate a click event on the checkbox.
> Assert that the checkbox has been checked (to catch, for example,
> the disabled case).
>  -- tomdale
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.

3 participants