Skip to content

refactor(cdk-experimental/testing): Refactor the ComponentHarness API #16234

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

Merged
merged 11 commits into from
Jun 18, 2019

Conversation

mmalerba
Copy link
Contributor

@mmalerba mmalerba commented Jun 7, 2019

Drawbacks to current API

  • QueryOptions is confusing, undefined, true, and false mean different things for allowNull. The concept of allowNull also doesn't really apply when getting a list of elements
  • The find method is confusing as it doesn't actually find anything, it returns a function to find things
  • The user is required to specify the host selector for the harness they want to load, even though the harness should always know what selector its looking for

New API

The new API combines the variaous load methods as well as the Locator class from the old API into a single class called HarnessEnvironment. The HarnessEnvironment implements two separate interfaces, one for test authors: HarnessLoader, and one for component harness authors: LocatorFactory. Both protractor and testbed have their own concrete implementations of HarnessEnvironment, but they cannot be constructed directly. Users are required to construct them via a create method which returns a HarnessLoader interface. This way only the methods that make sense for the test author are exposed to the test author.

The HarnessLoader provides methods to find new HarnessLoaders for sub-portions of the DOM and methods to create ComponentHarnesses by searching for the corresponding host selector under its root element. Each ComponentHarness class is required to specify its host selector, so the user doesn't have to.

The LocatorFactory is the interface to the HarnessEnvironent available when extending ComponentHarness. It gives harness authors methods to create locator functions for finding TestElements and ComponentHarnesses under its root element. It also provides a method to get a LocatorFactory rooted at the document root, this can be used instead of the global option from the old API.

The methods described above for finding HarnessLoaders ComponentHarnesses and locator functions come in 3 different flavors (required, optional, and all). These different variants should be used instead of the old allowNull syntax. There is currently no replacement for {allowNull: undefined} but if we feel its important we can add a default variant of the method as well.

Examples

Component harness author code

// Old
class MyHarness extends ComponentHarness {
  readonly header = this.find(MyHeaderHarness, 'my-header', {allowNull: false});
  readonly overlay = this.find('.my-overlay', {allowNull: false, global: true});
}

// New
class MyHarness extends ComponentHarness {
  readonly header = this.requiredLocator(MyHeaderHarness);
  readonly overlay = this.documentRootLocatorFactory().optionalLocator('.my-overlay');
}

Test author code

// Old
it('does stuff', async () => {
  const harness = load(MyHarness, fixture);
  // Not possible to load harness for specific subsection
});

// New
it('does stuff', async () => {
  const harness = await TestbedHarnessEnvironment.harnessForFixtureRoot(MyHarness, fixture);
  const subsectionLoader = await TestbedHarnessEnvironment.create(fixture).findRequired('.sub-section');
  const subsectionHarness = await subsectionLoader.requiredHarness(MyHarness);
});

Questions

  • Should we add a plain locator method that behaves the same as requiredLocator for unit tests but doesn't check element.isPresent in e2e tests?
  • Its slightly more verbose, but should we call the methods locatorForRequired? It reads a little nicer: this.locatorForRequired(MyCompHarness)

@mmalerba mmalerba added pr: merge safe target: patch This PR is targeted for the next patch release labels Jun 7, 2019
@mmalerba mmalerba requested review from jelbourn and crisbeto June 7, 2019 20:23
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 7, 2019
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

@mmalerba mmalerba force-pushed the harness-refact branch 3 times, most recently from 67bc253 to 575f013 Compare June 14, 2019 17:16
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jun 18, 2019
@jelbourn jelbourn merged commit 22268c3 into angular:master Jun 18, 2019
RudolfFrederiksen pushed a commit to RudolfFrederiksen/material2 that referenced this pull request Jun 21, 2019
andrewseguin pushed a commit that referenced this pull request Jul 2, 2019
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants