Skip to content
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

feat: adding a new matcher for shadow DOM toMatch #463

Merged
merged 4 commits into from Jan 18, 2022

Conversation

k-j-kim
Copy link
Contributor

@k-j-kim k-j-kim commented Jan 12, 2022

Summary

Test plan

  • toMatchInShadow.test.js that executes the same test cases as toMatch

@k-j-kim
Copy link
Contributor Author

k-j-kim commented Jan 12, 2022

Since I'm unsure whether it's best to create a separate matcher for this or to augment toMatch, I'm creating this PR to gather feedback.

@k-j-kim k-j-kim marked this pull request as ready for review January 13, 2022 01:42
@UziTech
Copy link
Contributor

UziTech commented Jan 13, 2022

It looks like it would be better to add an option to toMatch since this doesn't only match if the text is in shadow dom.

Also make sure to update notToMatch.js to make the .not.toMatch work.

@k-j-kim
Copy link
Contributor Author

k-j-kim commented Jan 13, 2022

fair point @UziTech, let me update the PR.

Comment on lines +53 to +55
const textContent = traverseShadowRoots
? getShadowTextContent(handle)
: handle.textContent
Copy link
Contributor Author

Choose a reason for hiding this comment

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

get textContent from either getShadowTextContent or node's textContent based on traverseShadowRoots option.

}
})
})
describe.each(['Page', 'Frame', 'ShadowPage', 'ShadowFrame'])(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same test cases, just adding page types.

Comment on lines +16 to +18
const options = ['ShadowPage', 'ShadowFrame'].includes(pageType)
? { traverseShadowRoots: true }
: {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

run toMatch with option if in shadow DOM page type

}
})

describe('ElementHandle', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved ElementHandle into the page type block. Was there a reason it was not tested for frames?

Copy link
Contributor

@UziTech UziTech left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@k-j-kim
Copy link
Contributor Author

k-j-kim commented Jan 17, 2022

tyvm for the review @UziTech! 🙏

is there anything else I need to do to get the PR ready for merge?

@UziTech UziTech merged commit 28c5235 into argos-ci:master Jan 18, 2022
@UziTech
Copy link
Contributor

UziTech commented Jan 18, 2022

Nope. I will release this tonight. Thanks!

@UziTech
Copy link
Contributor

UziTech commented Feb 2, 2022

Sorry for the delay. Finally got around to releasing 6.1.0 with this change 🎉

@k-j-kim k-j-kim deleted the tomatch-shadowdom branch February 3, 2022 19:32
@k-j-kim
Copy link
Contributor Author

k-j-kim commented Feb 3, 2022

thanks for all your help on this @UziTech!! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can not match text and elements in closed shadowDOM
2 participants