Skip to content

feat(material/tree): add getTreeStructure for tree harness #20568

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 14, 2020
Merged

feat(material/tree): add getTreeStructure for tree harness #20568

merged 3 commits into from
Oct 14, 2020

Conversation

annieyw
Copy link
Contributor

@annieyw annieyw commented Sep 15, 2020

Add a "pretty print" method for the tree harness.
The method is only available at the tree/root level because it is not possible to get children of flat nodes as they are siblings in the DOM.

@annieyw annieyw requested review from andrewseguin and a team as code owners September 15, 2020 01:11
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 15, 2020
@annieyw
Copy link
Contributor Author

annieyw commented Sep 15, 2020

Currently just using spaces to denote levels. Looking into other ascii representation. While more complicated ascii strings may be easier to read but may be hard for users to produce the correct string to assert on.

mmalerba
mmalerba reviewed Sep 15, 2020
View reviewed changes
mmalerba
mmalerba reviewed Sep 16, 2020
View reviewed changes
@annieyw annieyw requested a review from mmalerba September 16, 2020 19:26
mmalerba
mmalerba approved these changes Sep 16, 2020
View reviewed changes
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

LGTM, marking as blocked until pre-req PR goes in

@mmalerba mmalerba added blocked This issue is blocked by some external factor, such as a prerequisite PR target: minor This PR is targeted for the next minor release labels Sep 16, 2020
devversion
devversion reviewed Sep 16, 2020
View reviewed changes
src/material/tree/testing/tree-harness.ts Outdated
@@ -28,4 +28,17 @@ export class MatTreeHarness extends ComponentHarness {
async getNodes(filter: TreeNodeHarnessFilters = {}): Promise<MatTreeNodeHarness[]> {
return this.locatorForAll(MatTreeNodeHarness.with(filter))();
}

async getTreeStructure(): Promise<string> {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any motivation for adding this function? Is it to simplify test assertions for a tree and it's state? I'm wondering if it would be better type-wise and assertion-wise to just return a nested array or object instead.

@annieyw annieyw removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label Sep 29, 2020
@annieyw annieyw requested a review from mmalerba September 29, 2020 21:33
@annieyw
Copy link
Contributor Author

annieyw commented Sep 29, 2020

Changed to object representation of tree structure as asserting against a string can be finicky

@annieyw annieyw changed the title feat(tree/testing): add getTreeStructure for tree harness feat(material/tree): add getTreeStructure for tree harness Sep 29, 2020
@annieyw annieyw force-pushed the tree-pretty-print branch 2 times, most recently from 0a1967b to 5386107 Compare October 1, 2020 18:51
mmalerba
mmalerba reviewed Oct 1, 2020
View reviewed changes
src/material/tree/testing/tree-harness.ts Outdated
*/
async getTreeStructure(): Promise<TreeNode> {
const nodes = await this.getNodes();
const nodeInformation = await Promise.all(nodes.map(node => {
Copy link
Contributor

Choose a reason for hiding this comment

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

now that parallel is available from @angular/cdk/testing please use that over Promise.all

@annieyw annieyw force-pushed the tree-pretty-print branch from 5386107 to c2bb057 Compare October 2, 2020 15:39
mmalerba
mmalerba reviewed Oct 2, 2020
View reviewed changes
@annieyw annieyw force-pushed the tree-pretty-print branch from c2bb057 to 4684d5a Compare October 2, 2020 18:16
mmalerba
mmalerba reviewed Oct 3, 2020
View reviewed changes
@annieyw annieyw force-pushed the tree-pretty-print branch from 4684d5a to cdf3345 Compare October 5, 2020 17:15
mmalerba
mmalerba approved these changes Oct 5, 2020
View reviewed changes
Copy link
Contributor

@mmalerba mmalerba left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, please update the API golds and commit message then apply merge ready label

@annieyw annieyw force-pushed the tree-pretty-print branch from cdf3345 to c1279bd Compare October 6, 2020 15:13
@annieyw annieyw added action: merge The PR is ready for merge by the caretaker P2 The issue is important to a large percentage of users, with a workaround and removed action: merge The PR is ready for merge by the caretaker labels Oct 6, 2020
@annieyw annieyw merged commit 483c3e2 into angular:master Oct 14, 2020
@annieyw annieyw deleted the tree-pretty-print branch November 9, 2020 19:41
@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 Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@devversion devversion devversion left review comments

@mmalerba mmalerba mmalerba approved these changes

@jelbourn jelbourn Awaiting requested review from jelbourn

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@annieyw @devversion @mmalerba @googlebot