-
Notifications
You must be signed in to change notification settings - Fork 5
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
chore: remove Button's test helpers #257
Conversation
🦋 Changeset detectedLatest commit: ce91639 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
56b7fe3
to
915db1a
Compare
Preview URLsEnv: preview |
@ynotdraw: Before I take this out of draft, add a description, and send it out for review, I thought I'd see how you feel. I'm working on adding test helpers to components that need them, but I'm wondering: does Button need them? |
@@ -55,8 +49,6 @@ module('Integration | Component | button', function (hooks) { | |||
</Button> | |||
</template>); | |||
|
|||
assert.false(buttonPageObject.isLoading); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only test helper I was able to see a decent argument for. Is it something we need to test? Or does the [data-test-loading]
assertion below sufficiently cover it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel as though the assertion below is sufficient enough probably
915db1a
to
1121b7e
Compare
@@ -55,8 +49,6 @@ module('Integration | Component | button', function (hooks) { | |||
</Button> | |||
</template>); | |||
|
|||
assert.false(buttonPageObject.isLoading); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel as though the assertion below is sufficient enough probably
Good question. I'm kind of thinking maybe these aren't super helpful? All of this stuff can be accomplished already with the built-in ember qunit assertions and/or with the built-in ember test helpers. I think components like our autocomplete and multi select are complicated enough that they warrant helpers; however, I'm not sure I feel the same with A bit tangential to this, but I also wonder if we should make |
Sounds good. I'll move ahead with this PR.
🤔 |
1121b7e
to
ce91639
Compare
🚀 Description
I've been going through our components and adding test helpers so consumers don't have to select elements themselves, thereby digging into our implementation. It occurred to me while thinking about what needs test helpers and what doesn't that Button may not need any.
Because Button is more or less composed of one element (
<button />
) the consumer can spread an ID or data attribute onto Button and use that ID or data attribute to select that element. He can then use a combination of QUnit DOM and Ember Test Helpers to assert what needs asserting. No need, for example, to provide an abstraction for checking whether Button is disabled or for clicking it.🔬 How to Test
If the build is green, we're good.