Spike on assertContent test helper #146

ryanlabouve · 2016-12-23T07:24:39Z

This PR adds an assertContent(assert, contentString, options) test helper to allow assertion on the content of a tooltip.

This may be a basic implementation, but would love feedback on how to improve.

Also, I feel like I should add additional tests to cover passing in at least the selector options.

Is this heading in the right direction?

(This relates to #128)

sir-dunxalot · 2016-12-23T11:01:34Z

test-support/helpers/ember-tooltips.js

@@ -139,3 +139,14 @@ export function assertTooltipSide(assert, options = {}) {
      'Tooltip should be left of the target');
  }
 }
+
+export function assertContent(assert, contentString, options = {}) {


To match existing test helper conventions, we should name this helper assertTooltipContent.

Also, I wonder if contentString should be a part of options. For the spacing and side helpers I worked on, I passed all the options via the options block to standardize the params going to each helper.

sir-dunxalot · 2016-12-23T11:05:09Z

tests/integration/components/content-test.js

+  this.render(hbs`
+    <div>
+      :)
+      {{tooltip-on-element text='Smiley face'}}


A good addition to this test will be a test for the block form:

<div> :) {{#tooltip-on-element}} Smiley face {{/tooltip-on-element}} </div>

sir-dunxalot · 2016-12-23T11:06:59Z

README.md

@@ -420,6 +420,7 @@ This addon exposes testing helpers which can be used inside of the consuming app
 * `assertTooltipRendered(assert)`: asserts if the tooltip has been rendered. When enableLazyRendering is true the tooltip will only be rendered after the user has interacted with the $target element. A tooltip can be rendered but not visible.
 * `assertTooltipNotRendered(assert)`: asserts if the tooltip has not been rendered. When enableLazyRendering is true the tooltip will only be rendered after the user has interacted with the $target element.
 * `assertTooltipSide(assert, { side: 'right' }): asserts that the tooltip is shown on the correct side of the target. Additional options that can be passed are `selector` and `targetSelector`.
+* `assertContent(assert, contentString): asserts that the content of the tooltip matches a given string. 


Seems good enough of a description for now. I'm going to be revamping the test helper documentation (#144) to give each test helpers its own heading and section in the readme.

sir-dunxalot · 2016-12-23T11:10:34Z

Great timing! This looks like a great start and it may well be this simple.

One general improvement will be to use this test helper throughout the tooltip where we previously tested content with out. I found tooltip-on-element-test, tooltip-on-component-test, and popover-on-component-test that should all be updated to use this new test helper. A quick search for trim() might reveal others.

ryanlabouve · 2016-12-23T17:02:16Z

Cool! I made the changes to helper name and signature and added the block form helper.

One general improvement will be to use this test helper throughout the tooltip where we previously tested content with out.

Are you wanting this to happen in this PR or in the future?

sir-dunxalot · 2016-12-24T00:39:55Z

This PR please. It shouldn't be a big fix but will make sure our test suite is up-to-date with this new addition and that will also help to 'test' the test helper.

I believe this is the complete list of tests to update:

sir-dunxalot · 2016-12-24T00:40:46Z

Once those updates are done we are good to merge this and it will be a great and widely-used addition to this library!

ryanlabouve · 2016-12-29T03:26:41Z

I added the tests to the spots you mentioned.

Here are a few questions / possible changes that I would like to make pending your 👍:

allowing targetSelector to determine tooltip content via target (link to source)
Show assertion if contentString is not passed in as an argument. This gives users of this test selector direct feedback if they are missing this required arg.
Should we do something to enabled the test to work for lazyRendering? e.g. here

a15n · 2016-12-29T19:10:09Z

"Should we do something to enabled the test to work for lazyRendering?"

If a tooltip is not rendered then there will be no tooltip or tooltipText. I don't see how this test would work in that scenario. Am I understanding the question correctly?

ryanlabouve · 2016-12-30T05:52:56Z

Perhaps I am misunderstanding, but lazy rendering is just waiting to stick the tooltips in the DOM until they are needed?

If so, if I was doing an acceptance test I initially thought I would want to check their content regardless if they were rendered or not. But now that I'm thinking about it I guess that's a leaky abstraction.

a15n · 2016-12-30T16:12:28Z

You're correct about how lazy rendering works.

There is no way to check their content unless they are rendered. So if you wanted to check their content in your acceptance test you'd need to do one of these things...

initialize isShown=true
initialize with enableLazyRendering=false
triggerTooltipEvent($tooltipTarget, 'mouseenter')

sir-dunxalot · 2016-12-30T21:38:50Z

allowing targetSelector to determine tooltip content via target (link to source)

Could you expand on what capability you'd like to add?

Show assertion if contentString is not passed in as an argument. This gives users of this test selector direct feedback if they are missing this required arg.

All for this! 👍 To be consistent with existing tests, Ember.assert could be used (see example here).

sir-dunxalot · 2016-12-30T21:41:57Z

tests/integration/components/tooltip-on-element-test.js

@@ -41,7 +42,11 @@ test('tooltip-on-element has the proper aria-describedby tag', function(assert)
  const $tooltipTarget = this.$('.target');
  const describedBy = $tooltipTarget.attr('aria-describedby');

-  assert.equal(this.$(`#${describedBy}`).text().trim(), 'Some info in a tooltip.');
+  assertTooltipContent(assert, {
+    // targetSelector: '.target',


The discussion about this line needs to be resolve in the comments.

sir-dunxalot · 2016-12-30T21:45:24Z

One comment left to resolve (see prior comment) - otherwise the code changes look good!

sir-dunxalot · 2017-01-01T12:24:13Z

@ryanlabouve Looks like the latest changes to master branch have created conflicts with this branch.

This relates to sir-dunxalot#128

* Removed unneeded comment * Added assertion for if contentString is undefined

ryanlabouve · 2017-01-06T07:04:34Z

Rebased off of master and fixed conflicts!

Also added the assertion we talked about and removed an unneeded comment.

Would you like me to fix the new ESLint errors as well? They are not related to this PR but it looks like they are causing the tests to fail.

sir-dunxalot · 2017-01-06T12:40:11Z

Yes pleas, Ryan. Sorry for asking you to make unrelated changes. This is in part due to this PR being opened before a couple of bigger sweeping PRs were merged (addon linting being one of them).

ryanlabouve · 2017-01-06T15:31:49Z

Cool. Just pushed the linting changes.

sir-dunxalot · 2017-01-06T18:59:38Z

Great! I'll make a final review and should be able to approve changes before the end of this weekend.

This will be an awesome addition!

a15n · 2017-01-07T00:14:16Z

good work Ryan

ryanlabouve · 2017-01-07T00:30:02Z

Thanks!

a15n

overall it looks good. I left a few comments that will be easy to fix. Thanks again for doing this.

a15n · 2017-01-07T00:36:17Z

README.md

@@ -421,6 +421,7 @@ This addon exposes testing helpers which can be used inside of the consuming app
 * `assertTooltipNotRendered(assert)`: asserts if the tooltip has not been rendered. When enableLazyRendering is true the tooltip will only be rendered after the user has interacted with the $target element.
 * `assertTooltipSide(assert, { side: 'right' }): asserts that the tooltip is shown on the correct side of the target. Additional options that can be passed are `selector` and `targetSelector`.
 * `assertTooltipSpacing(assert, { side: 'right', spacing: 10 }): asserts that the tooltip is a given distance from the target (on a given side). `side` and `spacing` must be passed. Additional options that can be passed are `selector` and `targetSelector`.
+* `assertContent(assert, contentString): asserts that the content of the tooltip matches a given string. 


this needs to be updated...

assertTooltipContent(assert, { contentString: 'hello' }); .....

Good catch!

a15n · 2017-01-07T00:37:09Z

test-support/helpers/ember-tooltips.js

+export function assertTooltipContent(assert, options = {}) {
+  const { contentString } = options;
+
+  if (contentString === undefined) {


could you use http://emberjs.com/api/#method_isEmpty

If we use isEmpty then a user would not be able to test the case where contentString is an empty string.

ah I meant 'isNone'

a15n · 2017-01-07T00:38:42Z

test-support/helpers/ember-tooltips.js

+  const $tooltip = getTooltipFromBody(options.selector);
+  const tooltipContent = $tooltip.text().trim();
+
+  assert.equal(


nit: not blocking, could you follow the syntax we did elsewhere in this file...

assert.equal(a, b, 'some description');

PS: I like that you added the tooltipContent and contentString in the description

a15n · 2017-01-07T00:39:23Z

tests/integration/components/content-test.js

+
+  assert.expect(2);
+
+  this.render(hbs`


the div is unnecessary. Could be written like...

this.render(hbs`{{tooltip-on-element test="foo"}}`);

I think this makes the tests harder to read, but would be happy to change. My reasoning is that the surrounding div gives someone reading the test more context.

Again, happy to change, please just confirm.

@sir-dunxalot ?

To stay consistent with our other tests I think we should remove the <div> and use:

this.render(hbs`{{tooltip-on-element test="foo"}}`);

You got it. Updating now.

a15n · 2017-01-07T00:39:54Z

tests/integration/components/content-test.js

+    contentString: 'Smiley face',
+  });
+
+  this.render(hbs`


could be simplified too (no div)

a15n · 2017-01-07T00:40:03Z

tests/integration/components/content-test.js

+  assert.expect(2);
+
+  this.render(hbs`
+    <div>


a15n · 2017-01-07T00:41:42Z

tests/integration/components/content-test.js

+    </div>
+  `);
+
+  const stubbedAssert = {


can you explain what stubbedAssert does? I'm confused.

This just stubs the equal on assert.equal that is passed in to our helper. The value this adds is testing that our helper calls what we think it will with the values it should have.

sir-dunxalot

I don't see any blocking requests in the PR review though @a15n brings up some good points in his review. @ryanlabouve please let me know when this is ready for merge given @a15n's comments.

ryanlabouve · 2017-01-08T04:20:08Z

Updates made! Please merge when you are able to confirm :-D

a15n · 2017-01-08T05:30:26Z

LGTM

sir-dunxalot · 2017-01-08T18:18:23Z

Merged! Release to come shortly.

sir-dunxalot · 2017-01-08T23:01:03Z

Released as 2.8.0. Thanks a lot @ryanlabouve!

sir-dunxalot requested changes Dec 23, 2016

View reviewed changes

sir-dunxalot mentioned this pull request Dec 23, 2016

Add more test helpers #128

Closed

3 tasks

sir-dunxalot reviewed Dec 30, 2016

View reviewed changes

ryanlabouve added 4 commits January 6, 2017 00:42

Spike on assertContent test helper

a8426dd

This relates to sir-dunxalot#128

Update assertTooltipContent helper name and signature

19b8b67

Add block form test for assertTooltipContent

706ccde

Update helper to be used throughout current test suite

7262d71

ryanlabouve force-pushed the test-helper-assert-content branch from 5e1693b to 7262d71 Compare January 6, 2017 06:54

Address PR comments

220db86

* Removed unneeded comment * Added assertion for if contentString is undefined

yey linting

d3cca68

sir-dunxalot added the enhancement label Jan 6, 2017

a15n reviewed Jan 7, 2017

View reviewed changes

sir-dunxalot approved these changes Jan 7, 2017

View reviewed changes

PR Updates

d72bc25

sir-dunxalot merged commit f683efc into sir-dunxalot:master Jan 8, 2017

Spike on assertContent test helper #146

Spike on assertContent test helper #146

Conversation

ryanlabouve commented Dec 23, 2016 • edited by a15n Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sir-dunxalot commented Dec 23, 2016 • edited Loading

ryanlabouve commented Dec 23, 2016

sir-dunxalot commented Dec 24, 2016 • edited Loading

sir-dunxalot commented Dec 24, 2016

ryanlabouve commented Dec 29, 2016

a15n commented Dec 29, 2016

ryanlabouve commented Dec 30, 2016

a15n commented Dec 30, 2016

sir-dunxalot commented Dec 30, 2016 • edited Loading

sir-dunxalot Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

sir-dunxalot commented Dec 30, 2016

sir-dunxalot commented Jan 1, 2017

ryanlabouve commented Jan 6, 2017

sir-dunxalot commented Jan 6, 2017

ryanlabouve commented Jan 6, 2017

sir-dunxalot commented Jan 6, 2017

a15n commented Jan 7, 2017

ryanlabouve commented Jan 7, 2017

a15n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sir-dunxalot Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sir-dunxalot left a comment

Choose a reason for hiding this comment

ryanlabouve commented Jan 8, 2017

a15n commented Jan 8, 2017

sir-dunxalot commented Jan 8, 2017

sir-dunxalot commented Jan 8, 2017

ryanlabouve commented Dec 23, 2016 •

edited by a15n

Loading

sir-dunxalot commented Dec 23, 2016 •

edited

Loading

sir-dunxalot commented Dec 24, 2016 •

edited

Loading

sir-dunxalot commented Dec 30, 2016 •

edited

Loading

sir-dunxalot Dec 30, 2016 •

edited

Loading

sir-dunxalot Jan 7, 2017 •

edited

Loading