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

Add queryElement test helper function #1486

Merged
merged 2 commits into from
Aug 21, 2020
Merged

Add queryElement test helper function #1486

merged 2 commits into from
Aug 21, 2020

Conversation

spectranaut
Copy link
Contributor

This PR fulfills the request from issue #1403 to have a queryElement helper function that is analogous to queryElements. It also replaces all the calls of queryElements when only the first element is needed.

@jongund
Copy link
Contributor

jongund commented Aug 11, 2020

@spectranaut
How will this change effect current regression tests and the regression tests in pull requests?
Will there be linting to make using findElement failing?

Copy link
Contributor

@smhigley smhigley left a comment

Choose a reason for hiding this comment

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

Now I'm wondering if I should have named queryElements to make it more obvious that it failed if no elements were found, and to differentiate more meaningfully from findElement(s) 😅.

This looks fine though. Is it mostly just to provide parity with queryElements, so test authors aren't confused?

@jongund jongund self-requested a review August 19, 2020 15:57
Copy link
Contributor

@jongund jongund left a comment

Choose a reason for hiding this comment

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

How will this change effect current regression tests and the regression tests in pull request?
How will linting to make using findElement failing?

@spectranaut
Copy link
Contributor Author

@smhigley yup! just for parity.

@jongund thanks for asking but I didn't add any new lint errors, and you can still use findElement. The goal was just to provide an API like queryElements called queryElement, because authors might expect it.

@mcking65
Copy link
Contributor

OK, thank you @spectranaut and @smhigley, this seems ready to go. @jongund , it appears that it won't cause any disruption of work for you. I will merge.

Copy link
Contributor

@jongund jongund left a comment

Choose a reason for hiding this comment

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

Thank you for the status of how this change affects existing pull requests.

@jongund
Copy link
Contributor

jongund commented Aug 21, 2020

@spectranaut @smhigley
Is there plan to update all the regression tests to use only queryElements and queryElement?

@mcking65 mcking65 merged commit e8a984c into master Aug 21, 2020
@mcking65 mcking65 deleted the query-element branch August 21, 2020 22:32
michael-n-cooper pushed a commit that referenced this pull request Aug 21, 2020
Infrastructure: Add util queryElement function to regression test utils (pull #1486)

Fixes #1403 by adding a `queryElement` helper in /test/util.
This commit also replaces all the calls to `queryElements` with `queryElement` when only the first element is needed.
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.

None yet

4 participants