feat(cdk-experimental/testing): Bring in component harness #16089

mmalerba · 2019-05-22T17:13:32Z

This is the initial PR bringing in the the ComponentHarness from google3. Huge thanks to @qiyigg and @vikerman for getting this started, and to all who contributed to the code in google3.

Main files

cdk-experimental/testing/component-harness.ts contains classes and interfaces that define the test-environment-independent component harness
- ComponentHarness the base class for all harnesses
- TestElement an interface used by the ComponentHarness to interface with the DOM
- Locator an interface used by the ComponentHarness to find TestElements
cdk-experimental/testing/testbed.ts contains implementation specific to Angular's TestBed
- TestBed compatible implementations of TestElement and Locator
- load method used to create instances of ComponentHarness that are compatible with TestBed
- Depends on @angular/core/testing
cdk-experimental/testing/protractor.ts contains implementation specific to Protractor.
- Protractor compatible implementations of TestElement and Locator
- load method used to create instances of ComponentHarness that are compatible with TestBed
- Depends on protractor and selenium-webdriver
cdk-experimental/testing/tests/* contains tests for the component harness framework, including definitions for a couple simple components and corresponding harnesses to be used in the tests

Future work

Have the harness know it's component's root selector
Add convenience method for checking CSS classes
Fix protractor implementation of focs/blur
Add ability for testbed implementation to detect fakeAsync tests
Refactor names and structure of some of the objects (e.g. load)
Consider what other actions might need to be added to TestElement (e.g. tap)
The current allowNull behavior seems confusing, consider revising it
Consider separating methods for sending key presses and typing input
Consider moving protractor and testbed implementations to their own tertiary entry points

src/cdk-experimental/testing/component-harness.ts

crisbeto · 2019-05-24T16:58:55Z

src/cdk-experimental/testing/component-harness.ts

+   * Search all matched test elements under current root by css selector.
+   * @param css Selector of the test elements.
+   */
+  findAll(css: string): Promise<TestElement[]>;


Should we call these querySelector and querySelectorAll since it's what people are familiar with?

I changed the Locator methods to querySelector/querySelectorAll, it is a little bit different though because it returns a Promise. The methods of the same name on ComponentHarness are even more different because they return () => Promise. I've left them as find/findAll for now, but open to renaming those as well

src/cdk-experimental/testing/test-app/harnesses/main-component-harness.ts

src/cdk-experimental/testing/test-app/main-component.ts

crisbeto · 2019-05-24T17:11:34Z

src/cdk-experimental/testing/protractor.ts

+    return toPromise<string>(this.element.getCssValue(property));
+  }
+
+  async hover(): Promise<void> {


How would we handle this if the consumer was running the test against a touch screen device?

I don't think it would be an issue necessarily. Protractor must have some way to handle mouseMove on touch devices already, even if its to just do nothing. It would make sense to add a tap action as well. I can do that in a follow-up PR.

AFAIK Protractor simulates real user input at the OS level so I'm not sure how it work. My main concern with hover was for unit tests and it should be a bit easier to simulate it there though.

src/cdk-experimental/testing/testbed.ts

crisbeto · 2019-05-24T17:21:31Z

src/cdk-experimental/testing/testbed.ts

+    await this._stabilize();
+  }
+
+  async getCssValue(property: string): Promise<string> {


This could be flaky between browsers, because some of the normalize things like color values. IIRC Edge or IE return all colors in RGBA, regardless of the format the color was set in.

Maybe we can add normalization logic here as we encounter things that need it. In the mean time, the harness author can always normalize on their end if needed.

Also what do you think about the name of this method? Should I change it to getComputedStyle?

I don't feel strongly about the name. We could call it getPropertyValue which corresponds to the method on the CSSStyleDeclaration that gets returned from getComputedStyle.

src/cdk-experimental/testing/testbed.ts

crisbeto · 2019-05-24T17:25:16Z

src/cdk-experimental/testing/testbed.ts

+      this.element instanceof HTMLTextAreaElement)) {
+      throw new Error('Attempting to clear an invalid element');
+    }
+    this.element.focus();


Moving focus here might a bit risky since it's not what would necessarily happen to a real user. E.g. an input "clear" button won't move focus into the input.

Is that true? in Chrome with an <input type="search"> pressing the button also focuses.

I meant if it's a custom clear button, rather than the native browser one.

Ah, in that case the particular component that implements the clear button can choose to expose a clear method in its harness API that clicks the custom button instead. This class is meant to be an implementation of the interface harness authors use to write their harnesses.

src/cdk-experimental/testing/testbed.ts

src/cdk-experimental/testing/component-harness.ts

src/cdk-experimental/testing/testbed.ts

src/cdk-experimental/testing/component-harness.ts

src/cdk-experimental/testing/protractor.ts

jelbourn · 2019-05-24T20:53:37Z

src/cdk-experimental/testing/testbed.ts

+    await this._stabilize();
+    (this.element as HTMLElement).focus();
+    const e = this.element as HTMLInputElement;
+    for (const key of keys) {


Does this logic come from somewhere? It would be good to reference back to it

Not that I'm aware of, but Yi is the original author

src/cdk-experimental/testing/testbed.ts

mmalerba · 2019-06-03T18:31:42Z

@jelbourn @crisbeto I've addressed all of the comments above, left some of them unresolved for further dicsussion

Co-Authored-By: Yi Qi <qiyi@google.com> Co-Authored-By: Rado Kirov <radokirov@google.com> Co-Authored-By: Thomas Shafer <tshafer@google.com> Co-Authored-By: Caitlin Mott <caitlinmott@google.com> Co-Authored-By: Craig Nishina <cnishina@google.com>

crisbeto

Left a couple of nits, but aside from those it LGTM.

crisbeto · 2019-06-05T17:14:26Z

src/cdk-experimental/testing/component-harness.ts

+   * Search all matched test elements under current root by CSS selector.
+   * @param selector The CSS selector of the test elements.
+   */
+  querySelectorAll(selector: string): Promise<TestElement[]>;


Should querySelectorAll accept options as well?

I'm planning to address this when I make some API changes in the next PR. I think this is currently implemented like this because allowNull doesn't really make sense for querySelectorAll

src/cdk-experimental/testing/testbed.ts

jelbourn

LGTM, I'm good iterating on API/naming in follow-up PRs

Co-Authored-By: Yi Qi <qiyi@google.com> Co-Authored-By: Rado Kirov <radokirov@google.com> Co-Authored-By: Thomas Shafer <tshafer@google.com> Co-Authored-By: Caitlin Mott <caitlinmott@google.com> Co-Authored-By: Craig Nishina <cnishina@google.com>

…16089) Co-Authored-By: Yi Qi <qiyi@google.com> Co-Authored-By: Rado Kirov <radokirov@google.com> Co-Authored-By: Thomas Shafer <tshafer@google.com> Co-Authored-By: Caitlin Mott <caitlinmott@google.com> Co-Authored-By: Craig Nishina <cnishina@google.com>

angular-automatic-lock-bot · 2019-09-10T23:01:14Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

mmalerba requested review from jelbourn, crisbeto and andrewseguin May 22, 2019 17:13

mmalerba requested a review from devversion as a code owner May 22, 2019 17:13

mmalerba assigned jelbourn, crisbeto, andrewseguin and devversion May 22, 2019

mmalerba added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 22, 2019

Splaktar self-requested a review May 23, 2019 17:53

crisbeto reviewed May 24, 2019

View reviewed changes

jelbourn reviewed May 24, 2019

View reviewed changes

mmalerba force-pushed the harness branch 4 times, most recently from f97a374 to 2e31b2d Compare June 3, 2019 18:25

mmalerba added target: patch This PR is targeted for the next patch release pr: merge safe labels Jun 3, 2019

mmalerba force-pushed the harness branch from 2e31b2d to 96a3545 Compare June 3, 2019 18:29

mmalerba force-pushed the harness branch 2 times, most recently from 0a679f2 to 0914d84 Compare June 3, 2019 23:02

mmalerba and others added 7 commits June 5, 2019 08:33

make sure code compiles and runs

3ecb9fc

merge component harness test-app into e2e-app

84a9acf

address @crisbeto's feedback

ee81405

address @jelbourn's comments

ae85b37

fix bazel lint

3a6efc8

remove some TODOs that are no longer issues

a666f34

mmalerba force-pushed the harness branch from bbe403b to a666f34 Compare June 5, 2019 15:34

crisbeto approved these changes Jun 5, 2019

View reviewed changes

jelbourn approved these changes Jun 5, 2019

View reviewed changes

address latest round of feedback

8db9e6e

mmalerba force-pushed the harness branch from fef1ec8 to 8db9e6e Compare June 5, 2019 21:02

mmalerba added the action: merge The PR is ready for merge by the caretaker label Jun 5, 2019

josephperrott merged commit 7baac27 into angular:master Jun 5, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019

feat(cdk-experimental/testing): Bring in component harness #16089

feat(cdk-experimental/testing): Bring in component harness #16089

Uh oh!

Conversation

mmalerba commented May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Main files

Future work

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

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!

mmalerba May 31, 2019 • 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!

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!

Uh oh!

Uh oh!

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!

Uh oh!

mmalerba commented Jun 3, 2019

Uh oh!

crisbeto left a comment

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!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 10, 2019

Uh oh!

Uh oh!

mmalerba commented May 22, 2019 •

edited

Loading

mmalerba May 31, 2019 •

edited

Loading