Additional TooltipTrigger Tests #193

MidnightCoder06 · 2020-02-12T00:50:41Z

Closes https://jira.corp.adobe.com/browse/RSP-1512

adobe-bot · 2020-02-12T00:52:11Z

Build successful! View the storybook

packages/@react-stately/tooltip/test/TooltipManager.test.js

adobe-bot · 2020-02-18T00:40:27Z

Build successful! View the storybook

packages/@react-spectrum/tooltip/test/TooltipTrigger.test.js

packages/@react-aria/interactions/test/DOMPropsResponder.test.js

packages/@react-spectrum/tooltip/test/TooltipTrigger.test.js

packages/@react-aria/interactions/test/useHover.test.js

adobe-bot · 2020-02-23T22:46:15Z

Build successful! View the storybook

…tipTriggerTests

snowystinger · 2020-02-24T18:41:33Z

packages/@react-aria/interactions/test/DOMPropsResponder.test.js

+describe('DOMPropsResponder', function () {
+  afterEach(cleanup);
+
+  it('should handle hover events on nested hoverable children', function () {


I don't understand this name, I don't see any nested Hoverable components. Should I be seeing any more than the one?

snowystinger · 2020-02-24T18:44:59Z

packages/@react-aria/interactions/test/DOMPropsResponder.test.js

+import {DOMPropsResponder, Hoverable} from '../';
+import React from 'react';
+
+describe('DOMPropsResponder', function () {


I don't understand how this is a DOMPropsResponder test suite, nothing in here references it other than it being the wrapping component. Is there anything we should be checking on that component itself during our tests?
Otherwise it just looks like the test suite directly below about Hoverable

+1 to the above ^

snowystinger · 2020-02-24T19:12:38Z

packages/@react-spectrum/tooltip/test/TooltipTrigger.test.js

        </Provider>
      );

-      expect(() => {


why was this removed?

is this because of this comment?

Still not sure why the closing actions aren't being registered in tooltiptrigger tests file 🤔 so I deleted them instead of doing the id checking hack. Technically this is handled with the tooltip manager tests file anyway 🤷‍♂

Lets try to figure out why, I have some time today, maybe we can look at it together after lunch?

I just tried adding it back and it caused a failure. Not sure why, it was there for quite some time and I don't see what has changed.

snowystinger · 2020-02-24T19:13:50Z

packages/@react-spectrum/tooltip/test/TooltipTrigger.test.js

  describe('click related tests', function () {

-    it('triggered by click event', async function () {
+    it('a click event can open the tooltip', async function () {


what's the difference between this test and the one above it?

In the first one I am checking Id's (line 59 and and 60). In the one you referenced here I am not

Right, so why do we need this test, what is the value that this test adds over the previous one?

packages/@react-spectrum/tooltip/test/TooltipTrigger.test.js

snowystinger · 2020-02-24T19:27:28Z

packages/@react-stately/tooltip/test/TooltipManager.test.js

+    expect(tooltipManager.hoverHideTimeout).toBeNull();
+
+    // run past first tooltips show timer
+    jest.advanceTimersByTime(150);


i think there should be a check for something right after this line?
originally I'd checked
expect(tooltipManager.visibleTooltips).toBeNull();
is there a reason for not checking this?

I don't see the line added?

packages/@react-stately/tooltip/test/TooltipManager.test.js

snowystinger · 2020-02-24T19:29:21Z

packages/@react-stately/tooltip/test/TooltipManager.test.js

@@ -0,0 +1,137 @@
+import {TooltipManager} from '../';
+
+describe('TooltipManager', () => {


I'm assuming that the remainder of the tests will be added in https://jira.corp.adobe.com/browse/RSP-1471
Can you please make notes of places that you diverged from the tests outlined there vs here and why?

That's a toast container, not a tooltip ticket?

yup, whoops, copy pasta mistake https://jira.corp.adobe.com/browse/RSP-1367

LFDanLu · 2020-02-24T19:57:23Z

packages/@react-aria/interactions/test/DOMPropsResponder.test.js

+import {DOMPropsResponder, Hoverable} from '../';
+import React from 'react';
+
+describe('DOMPropsResponder', function () {


+1 to the above ^

LFDanLu · 2020-02-24T20:02:13Z

packages/@react-aria/interactions/test/DOMPropsResponder.test.js

+      <DOMPropsResponder>
+        <div>
+          <Hoverable onHover={onHover}>
+            <button>Button</button>
+          </Hoverable>
+        </div>
+      </DOMPropsResponder>


Ah I think I understand what you are trying to test here now, these are like the PressResponder tests yeah? You should probably move the onHover and ref props to DOMPropsResponder so it actually tests that the DOMPropsResponder context is being set up properly and being consumed by Hoverable.

<DOMPropsResponder onHover={onHover}> ....

adobe-bot · 2020-02-24T23:07:26Z

Build successful! 🎉

adobe-bot · 2020-02-24T23:16:51Z

Build successful! 🎉

adobe-bot · 2020-02-25T00:46:34Z

Build successful! 🎉

adobe-bot · 2020-02-28T22:13:04Z

Build successful! 🎉

adobe-bot · 2020-02-28T22:22:34Z

Build successful! 🎉

LFDanLu

LGTM

codecov · 2020-02-28T23:23:08Z

Codecov Report

Merging #193 into master will increase coverage by 1.1%.
The diff coverage is 73.33%.

@@            Coverage Diff            @@
##           master     #193     +/-   ##
=========================================
+ Coverage   69.55%   70.65%   +1.1%     
=========================================
  Files         206      206             
  Lines        3961     3963      +2     
  Branches      839      842      +3     
=========================================
+ Hits         2755     2800     +45     
+ Misses        625      589     -36     
+ Partials      581      574      -7

Impacted Files	Coverage Δ
...s/@react-spectrum/menu-trigger/src/MenuTrigger.tsx	`93.33% <ø> (ø)`	⬆️
packages/@react-aria/menu-trigger/src/useMenu.ts	`72.72% <ø> (ø)`	⬆️
...ckages/@react-aria/menu-trigger/src/useMenuItem.ts	`54.16% <ø> (ø)`	⬆️
packages/@react-spectrum/menu-trigger/src/Menu.tsx	`85.71% <ø> (+0.52%)`	⬆️
...ckages/@react-spectrum/menu-trigger/src/context.ts	`100% <ø> (ø)`	⬆️
...ges/@react-aria/menu-trigger/src/useMenuTrigger.ts	`91.3% <100%> (+0.39%)`	⬆️
...react-stately/collections/src/CollectionBuilder.ts	`44.44% <50%> (+0.09%)`	⬆️
...eact-aria/selection/src/useSelectableCollection.ts	`52% <75%> (+3.13%)`	⬆️
...ges/@react-spectrum/tooltip/src/TooltipTrigger.tsx	`92.85% <0%> (+7.14%)`	⬆️
...kages/@react-aria/tooltip/src/useTooltipTrigger.ts	`88.46% <0%> (+11.53%)`	⬆️
... and 5 more

adobe-bot · 2020-02-28T23:23:58Z

Build successful! 🎉

Jean Leconte II added 3 commits February 10, 2020 17:53

saving progress

61aff7c

saving progress

c606e9f

saving progress

ba3f694

MidnightCoder06 requested review from dannify, devongovett and snowystinger as code owners February 12, 2020 00:50

MidnightCoder06 commented Feb 12, 2020

View reviewed changes

packages/@react-stately/tooltip/test/TooltipManager.test.js Outdated Show resolved Hide resolved

snowystinger reviewed Feb 12, 2020

View reviewed changes

packages/@react-stately/tooltip/test/TooltipManager.test.js Outdated Show resolved Hide resolved

Jean Leconte II added 3 commits February 17, 2020 09:40

tooltip manager tests done

ed33180

tooltip trigger tests done

92bded6

overcoming testing lib weirdness

86d7755

MidnightCoder06 changed the title ~~[WIP] Additional TooltipTrigger Tests~~ Additional TooltipTrigger Tests Feb 18, 2020

MidnightCoder06 commented Feb 18, 2020

View reviewed changes

packages/@react-spectrum/tooltip/test/TooltipTrigger.test.js Outdated Show resolved Hide resolved

MidnightCoder06 commented Feb 18, 2020

View reviewed changes

packages/@react-spectrum/tooltip/test/TooltipTrigger.test.js Outdated Show resolved Hide resolved

LFDanLu requested changes Feb 19, 2020

View reviewed changes

Jean Leconte II added 3 commits February 23, 2020 13:54

description name change

ef51c3a

added test for disable prop

4ecec14

remove tooltip closing tests

60a1222

Merge branch 'master' of github.com:adobe/react-spectrum-v3 into tool…

027bac0

…tipTriggerTests

snowystinger reviewed Feb 24, 2020

View reviewed changes

LFDanLu requested changes Feb 24, 2020

View reviewed changes

visible -> document

fbed664

MidnightCoder06 force-pushed the tooltipTriggerTests branch from 1154465 to fbed664 Compare February 24, 2020 23:09

ammend a fsm test

44b643a

Jean Leconte II added 7 commits February 26, 2020 17:30

waitForDOMChange -> await wait

423455c

close event tests work

0f94004

timers to replace promises testing done

525a63e

remove id check test

f45dd39

saving attempts at domprops responder tests

9e581ed

delete failed files

92758c1

make lint happy

22d0182

Merge branch 'master' into tooltipTriggerTests

b308b4c

snowystinger approved these changes Feb 28, 2020

View reviewed changes

LFDanLu approved these changes Feb 28, 2020

View reviewed changes

Merge branch 'master' into tooltipTriggerTests

b3e1db2

MidnightCoder06 merged commit 451be52 into master Feb 28, 2020

MidnightCoder06 deleted the tooltipTriggerTests branch February 28, 2020 23:25

		@@ -0,0 +1,137 @@
		import {TooltipManager} from '../';

		describe('TooltipManager', () => {

Additional TooltipTrigger Tests #193

Additional TooltipTrigger Tests #193

Conversation

MidnightCoder06 commented Feb 12, 2020

Uh oh!

adobe-bot commented Feb 12, 2020

Uh oh!

Uh oh!

Uh oh!

adobe-bot commented Feb 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adobe-bot commented Feb 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MidnightCoder06 Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MidnightCoder06 Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Feb 24, 2020

Uh oh!

adobe-bot commented Feb 24, 2020

Uh oh!

adobe-bot commented Feb 25, 2020

Uh oh!

adobe-bot commented Feb 28, 2020

Uh oh!

adobe-bot commented Feb 28, 2020

Uh oh!

LFDanLu left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

adobe-bot commented Feb 28, 2020

Uh oh!

MidnightCoder06 Feb 24, 2020 •

edited

Loading

MidnightCoder06 Feb 24, 2020 •

edited

Loading

codecov bot commented Feb 28, 2020 •

edited

Loading