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

Continue to flesh out more tests for new API's. #224

Merged
merged 4 commits into from
Oct 17, 2017

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Oct 17, 2017

Carries over many tests from the previous (now "legacy-0-6-x" suite) into the new system. This is largely to ensure that the regressions previously caught by these tests, are still checked for long after the legacy-0-6-x suite is gone.

Carries over many tests from the previous (now "legacy-0-6-x" suite)
into the new system. This is largely to ensure that the regressions
previously caught by these tests, are still checked for long after
the legacy-0-6-x suite is gone.
let subject = context.owner.lookup('service:Foo');

assert.equal(subject.someMethod(), 'hello thar!');
});
Copy link
Member

Choose a reason for hiding this comment

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

should we test this.inject('foo') here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, that API is no longer present (see examples and reasoning in the RFC), but I will add an example to both these tests and the rendering tests that will be a better demo of the go forward way to do it...

})
);

let subject = context.owner.lookup('service:Foo');
Copy link
Member

Choose a reason for hiding this comment

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

I suppose the uppercase Foo was deliberate? could probably use a comment in that case

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha, nope, not intentional. Will fix.

await this.render(hbs`{{x-foo}}`);

assert.equal(this.element.textContent, 'Click me!', 'precond - component was rendered');
click(this.element.querySelector('button'));
Copy link
Member

Choose a reason for hiding this comment

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

I assume at some point passing in just a CSS selector will also be supported?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya, these event helpers are only internal test things for now. My next RFC will be for adding the real ones here (and they will essentially have the same API as ember-native-dom-helpers).

Copy link
Member

Choose a reason for hiding this comment

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

would that mean deprecating ember-native-dom-helpers?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don’t know, but possibly. The main differences from what we need here is that we do not want to defer to Ember’s own internal application test globals. It’s definitely possible to do this transition a number of ways...

input.value = '1';

fireEvent(input, 'change');
assert.equal(this.get('value'), '1');
Copy link
Member

Choose a reason for hiding this comment

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

precondition assertion might make sense here

await this.clearRender();

assert.equal(this.get('foo'), 'updated!');
assert.equal(this.get('bar'), 'updated bar!');
Copy link
Member

Choose a reason for hiding this comment

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

why two properties? because one of them is rendered and the other is not? might need a comment 🤔

Copy link
Member Author

@rwjblue rwjblue Oct 17, 2017

Choose a reason for hiding this comment

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

Comment added...

@rwjblue rwjblue merged commit 79af2d8 into emberjs:master Oct 17, 2017
@rwjblue rwjblue deleted the add-more-tests-for-new-api branch October 17, 2017 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants