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

Expose importable helper functions. #222

Merged
merged 2 commits into from
Oct 17, 2017

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Oct 16, 2017

These functions can be used instead of repeating this. throughout the testing code.

Example:

import { module, test } from 'qunit';
import { setupRenderingTest, render } from 'ember-qunit';
import hbs from 'htmlbars-inline-precompile';

module('x-foo', function(hooks) {
  setupRenderingTest(hooks);

  test('renders', async function(assert) {
    assert.expect(1);

    await render(hbs`{{pretty-color name="red"}}`);

    assert.equal(this.element.querySelector('.color-name').textContent, 'red');
  });
});

This is essentially the compromise made with the rest of the core team while working on the the "Grand Unified Testing RFC". The helpers will continue to live on this (for easy discoverability), but are also importable (for cleaner looking tests). Using the non this. form of helpers obviously means that we cannot run concurrent tests, but that is extremely unlikely to work anyways.

@Turbo87
Copy link
Member

Turbo87 commented Oct 16, 2017

What were the arguments leading to this decision?

@rwjblue
Copy link
Member Author

rwjblue commented Oct 16, 2017

@Turbo87 roughly that the following "rightward" shift is something we can avoid:

test('foo', function(assert) {
  await this.visit('/some-path');
  await this.click('.some-link');
  await this.fillIn('.other-thing', 'derpy');
  await this.click('.submit');
  
   // assertions here
});

In the new system we are adding both await and this. which essentially shifts all lines 10 chars rightward.

Exposing the importable helpers (that simply defer to the this.* methods as in this PR) allows for the following API, while still being much less magical than today's acceptance test helpers:

import {
  visit,
  click,
  fillIn
} from 'ember-qunit';

// ...snip...
test('foo', function(assert) {
  await visit('/some-path');
  await click('.some-link');
  await fillIn('.other-thing', 'derpy');
  await click('.submit');
  
  // assertions here
});

@rwjblue
Copy link
Member Author

rwjblue commented Oct 16, 2017

I was a strong advocate for leaving the helper methods on the test context for discoverability, others were strongly opposed to the aesthetic of the rightward shift (as mentioned above). So I proposed this compromise (which satisfies both camps)...

@Turbo87
Copy link
Member

Turbo87 commented Oct 17, 2017

While render and clearRender make sense to me I'm not yet convinced about the element binding. In this case it has been this.element before already so the right shift argument doesn't count here, and consistency isn't given either because element is a binding/variable/property/whatever, not a helper function. And if we start to expose basically any property on the context then at some point we'll end up with a global owner, a global get(), a global set(), ...

@rwjblue
Copy link
Member Author

rwjblue commented Oct 17, 2017

Sounds good. I’ll remove element.

@rwjblue
Copy link
Member Author

rwjblue commented Oct 17, 2017

We could also expose element as a function (testElement() or getElement() or something), but I’m totally fine with punting in that for now.

These functions can be used instead of repeating `this.` throughout the testing code.

Example:

```js
import { module, test } from 'qunit';
import { setupRenderingTest, render, element } from 'ember-qunit';
import hbs from 'htmlbars-inline-precompile';

module('x-foo', function(hooks) {
  setupRenderingTest(hooks);

  test('renders', async function(assert) {
    assert.expect(1);

    await render(hbs`{{pretty-color name="red"}}`);

    assert.equal(element.querySelector('.color-name').textContent, 'red');
  });
});
```
Might bring it back later in a different incarnation, but removing for now.
@rwjblue rwjblue requested review from Turbo87 and removed request for Turbo87 October 17, 2017 13:32
@rwjblue rwjblue merged commit a9c39d9 into emberjs:master Oct 17, 2017
@rwjblue rwjblue deleted the importable-helpers branch October 17, 2017 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants