Add queryElements test helper, remove t.plan #1343

smhigley · 2020-02-24T07:45:02Z

Resolves #1312

Adds t.context.queryElements helper function that wraps t.context.session.findElements, and fails the test if no elements are found.

Removes t.plan(), now that it isn't needed to ensure that tests are run.

Removes some unrelated unused imports caught while updating the above.

zcorpan

Do we want to add a lint check to make sure t.plan() and findElements() aren't accidentally added in the future?

Edit: or, maybe not lint against t.plan(); there may be cases where it's still useful.

test/util/queryElements.js

zcorpan

I think a lint check would be good. We also need to update our documentation in the wiki for this change.

zcorpan · 2020-03-04T14:56:41Z

I added a lint check in c16088f -- there were still 12 uses (not counting the one in queryElement.js). Do you think those should be changed to use queryElements?

/Users/zcorpan/git/w3c/aria-practices/test/index.js
  89:16  error  'findElements' is restricted from being used. Please use t.context.queryElements()  no-restricted-properties

/Users/zcorpan/git/w3c/aria-practices/test/tests/checkbox_checkbox-2.js
   55:14  error  'findElements' is restricted from being used. Please use t.context.queryElements()  no-restricted-properties
   85:12  error  'findElements' is restricted from being used. Please use t.context.queryElements()  no-restricted-properties
  201:12  error  'findElements' is restricted from being used. Please use t.context.queryElements()  no-restricted-properties

/Users/zcorpan/git/w3c/aria-practices/test/tests/combobox_autocomplete-both.js
  60:31  error  'findElements' is restricted from being used. Please use t.context.queryElements()  no-restricted-properties

/Users/zcorpan/git/w3c/aria-practices/test/tests/combobox_autocomplete-list.js
  61:31  error  'findElements' is restricted from being used. Please use t.context.queryElements()  no-restricted-properties

/Users/zcorpan/git/w3c/aria-practices/test/tests/combobox_autocomplete-none.js
  61:31  error  'findElements' is restricted from being used. Please use t.context.queryElements()  no-restricted-properties

/Users/zcorpan/git/w3c/aria-practices/test/tests/combobox_grid-combo.js
  137:31  error  'findElements' is restricted from being used. Please use t.context.queryElements()  no-restricted-properties

/Users/zcorpan/git/w3c/aria-practices/test/tests/grid_LayoutGrids.js
  125:16  error  'findElements' is restricted from being used. Please use t.context.queryElements()  no-restricted-properties

/Users/zcorpan/git/w3c/aria-practices/test/tests/tabs_tabs-1.js
  355:12  error  'findElements' is restricted from being used. Please use t.context.queryElements()  no-restricted-properties

/Users/zcorpan/git/w3c/aria-practices/test/tests/tabs_tabs-2.js
  363:12  error  'findElements' is restricted from being used. Please use t.context.queryElements()  no-restricted-properties

/Users/zcorpan/git/w3c/aria-practices/test/util/assertAriaDescribedby.js
  51:36  error  'findElements' is restricted from being used. Please use t.context.queryElements()  no-restricted-properties

✖ 12 problems (12 errors, 0 warnings)

zcorpan

I've addressed my own comments. :-)

zcorpan · 2020-03-06T13:46:28Z

Documentation in the wiki should be updated. I don't want to do this now since my changes haven't been reviewed and I don't want the documentation to be out of sync with what's in master.

The eslint for findElements, and the per-file override /* eslint no-restricted-properties: 0 */
How to declare a test
Example test
Selenium API

mcking65 · 2020-03-09T05:21:44Z

@smhigley, could you please look at Simon's changes?

It would be good to have a review of the entire PR from @sh0ji or @spectranaut as well.

Given the number of files touched, I'd like to get this merged as soon as we can.

nschonni · 2020-03-09T06:25:18Z

.eslintrc.json

@@ -5,6 +5,10 @@
  },
  "rules": {
    "no-unused-vars": 0,
-    "no-undef": 0
+    "no-undef": 0,
+    "no-restricted-properties": [2, {


Should plan be added if you're trying to discourage it too?

nschonni · 2020-03-09T06:26:04Z

test/index.js

@@ -1,3 +1,5 @@
+/* eslint no-restricted-properties: 0 */


Might makes sense to use a single line ignore instead of a file level one

Looks like it can be re-enabled now

spectranaut

This looks great! but I don't think we can merge until this issue Simon brought up is resolved.

Simon said:

I added a lint check in c16088f -- there were still 12 uses (not counting the one in queryElement.js). Do you think those should be changed to use queryElements?

These would take more work to replace, because some of them make sure we get zero results from the query (which would cause a failure). I think our options are to (1) allow findElements in linter because of this case or (2) add a helper function for this particular scenario, and otherwise use queryElements. Preferences, @zcorpan or @smhigley ?

smhigley · 2020-03-09T19:44:00Z

I left those 12 in there intentionally because they dealt with testing the number returned, rather than just assuming within the test that there would be a non-empty array. I think that could still be a valid use case for findElements instead of queryElements.

What if we add a lint rule for t.plan instead, as @nschonni recommended? If that seems like it increases complexity too much for people writing tests, I also like @spectranaut's suggestion of a helper for that specific scenario. Perhaps something like assertNotEmpty or assertNumberOfElements (I'm bad at naming things... 😝)

zcorpan · 2020-03-10T11:04:52Z

I considered linting plan but wasn't sure if there are still cases where it's needed. If there aren't then we can lint it.

For the cases where findElements is still used, we want to assert that the length is 0? A separate helper function for that which fails if the length is not 0 seems cleanest to me. assertNoElement() maybe?

mcking65 · 2020-03-17T04:46:43Z

What is the best approach to arriving at a decision for how to resolve the remaining issue here? The technical aspect of this discussion is beyond my expertise.

One thing I hope we optimize for is clarity of code, especially for new comers to test writing.

smhigley · 2020-03-19T17:15:54Z

I like @zcorpan's suggestion of assertNoElements. I'll push up a commit with it in just a bit :)

smhigley · 2020-03-20T17:29:25Z

I rebased and removed all the remaining uses of findElements, apart from the two in the queryElements and assertNoElements helpers.

@spectranaut / @zcorpan / @nschonni, how do you feel about the PR after those changes?

nschonni

A few minor indenting things, maybe need to run eslint fix for.
Didn't go through them all, but might want to check that the /* eslint no-restricted-properties: 0 */ lines still make sense

nschonni · 2020-03-20T17:40:05Z

test/index.js

@@ -1,3 +1,5 @@
+/* eslint no-restricted-properties: 0 */


Looks like it can be re-enabled now

nschonni · 2020-03-20T17:48:49Z

test/tests/checkbox_checkbox-2.js

@@ -1,10 +1,13 @@
+/* eslint no-restricted-properties: 0 */


Think this can be re-enabled again

smhigley · 2020-03-20T22:20:24Z

@nschonni I removed the now-unnecessary eslint no-restricted-properties. eslint . --fix doesn't seem to be catching or changing anything, though

nschonni · 2020-03-20T23:50:55Z

@smhigley don't worry about it then :)
Looks like some mixed 2/4 spaces in some functions, but if the lint isn't catching it then the rules aren't enforcing it right now and shouldn't matter here

smhigley · 2020-04-03T18:25:47Z

@spectranaut are you ok with the PR now/do you have any outstanding concerns?

zcorpan

Reviewing on behalf of @spectranaut :)

LGTM. This now has conflicts though.

zcorpan · 2020-04-30T13:46:05Z

(I didn't get to resolving conflicts today.)

smhigley · 2020-05-03T06:39:12Z

@zcorpan conflicts resolved :)

zcorpan

Looks good, but a test in CI failed.

not ok 629 - NoSuchSessionError: Tried to run command without establishing a connection

Maybe this was a flaky failure? If so, we can try rerunning CI. (Done by closing and reopening.)

Infrastructure: Add queryElements test helper, remove t.plan from regression tests (pull #1343) Resolves issue #1312 with changes that: * add queryElements helper * update findElements to queryElements in tests, remove unused imports * update missing awaits * Add a lint check for findElements and exclude current uses * Document `t` argument, unconditionally return `result` * add assertNoElements test helper * removed unnecessary eslit no-restricted-props * update carousel tests to use queryElements Co-authored-by: Simon Pieters <zcorpan@gmail.com> Co-authored-by: Matt King <a11yThinker@Gmail.com>

…ression tests (pull #1343) Resolves issue #1312 with changes that: * add queryElements helper * update findElements to queryElements in tests, remove unused imports * update missing awaits * Add a lint check for findElements and exclude current uses * Document `t` argument, unconditionally return `result` * add assertNoElements test helper * removed unnecessary eslit no-restricted-props * update carousel tests to use queryElements Co-authored-by: Simon Pieters <zcorpan@gmail.com> Co-authored-by: Matt King <a11yThinker@Gmail.com>

smhigley force-pushed the 1312-plan-helper branch from 4e82cd4 to 87954aa Compare February 24, 2020 07:56

smhigley requested a review from spectranaut February 25, 2020 06:42

zcorpan mentioned this pull request Mar 3, 2020

Combobox examples with listbox popup: Add properties and states to listbox control button #1336

Merged

6 tasks

zcorpan self-requested a review March 3, 2020 19:21

zcorpan reviewed Mar 3, 2020

View reviewed changes

test/util/queryElements.js Outdated Show resolved Hide resolved

test/util/queryElements.js Outdated Show resolved Hide resolved

zcorpan requested changes Mar 3, 2020

View reviewed changes

zcorpan approved these changes Mar 6, 2020

View reviewed changes

mcking65 force-pushed the 1312-plan-helper branch from 8ea952a to e6a221e Compare March 9, 2020 05:24

nschonni reviewed Mar 9, 2020

View reviewed changes

spectranaut requested changes Mar 9, 2020

View reviewed changes

smhigley force-pushed the 1312-plan-helper branch from e6a221e to d614c2f Compare March 20, 2020 17:24

nschonni reviewed Mar 20, 2020

View reviewed changes

zcorpan requested a review from spectranaut April 30, 2020 13:18

zcorpan approved these changes Apr 30, 2020

View reviewed changes

smhigley and others added 4 commits May 2, 2020 23:19

add queryElements helper

d2bb414

update findElements to queryElements in tests, remove unused imports

6cf3701

update missing awaits

ef36227

Add a lint check for findElements and exclude current uses

6955c1d

zcorpan and others added 4 commits May 2, 2020 23:23

Document t argument, unconditionally return result

708c1e1

add assertNoElements test helper

7612c30

removed unnecessary eslit no-restricted-props

666837f

update carousel tests to use queryElements

0f80db3

smhigley force-pushed the 1312-plan-helper branch from 4fd14ca to 0f80db3 Compare May 3, 2020 06:38

zcorpan approved these changes May 4, 2020

View reviewed changes

zcorpan closed this May 4, 2020

zcorpan reopened this May 4, 2020

Merge branch 'master' into 1312-plan-helper

dd5b289

mcking65 merged commit b9f352d into master May 5, 2020

mcking65 deleted the 1312-plan-helper branch May 5, 2020 08:15

zcorpan mentioned this pull request May 14, 2020

Add queryElement (singular) API in addition to queryElements #1403

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add queryElements test helper, remove t.plan #1343

Add queryElements test helper, remove t.plan #1343

smhigley commented Feb 24, 2020

zcorpan left a comment •

edited

Loading

zcorpan left a comment

zcorpan commented Mar 4, 2020

zcorpan left a comment

zcorpan commented Mar 6, 2020

mcking65 commented Mar 9, 2020

nschonni Mar 9, 2020

nschonni Mar 9, 2020

nschonni Mar 20, 2020

spectranaut left a comment

smhigley commented Mar 9, 2020 •

edited

Loading

zcorpan commented Mar 10, 2020

mcking65 commented Mar 17, 2020

smhigley commented Mar 19, 2020

smhigley commented Mar 20, 2020

nschonni left a comment

nschonni Mar 20, 2020

nschonni Mar 20, 2020

smhigley commented Mar 20, 2020

nschonni commented Mar 20, 2020

smhigley commented Apr 3, 2020

zcorpan left a comment •

edited

Loading

zcorpan commented Apr 30, 2020

smhigley commented May 3, 2020

zcorpan left a comment •

edited

Loading

Add queryElements test helper, remove t.plan #1343

Add queryElements test helper, remove t.plan #1343

Conversation

smhigley commented Feb 24, 2020

zcorpan left a comment • edited Loading

Choose a reason for hiding this comment

zcorpan left a comment

Choose a reason for hiding this comment

zcorpan commented Mar 4, 2020

zcorpan left a comment

Choose a reason for hiding this comment

zcorpan commented Mar 6, 2020

mcking65 commented Mar 9, 2020

nschonni Mar 9, 2020

Choose a reason for hiding this comment

nschonni Mar 9, 2020

Choose a reason for hiding this comment

nschonni Mar 20, 2020

Choose a reason for hiding this comment

spectranaut left a comment

Choose a reason for hiding this comment

smhigley commented Mar 9, 2020 • edited Loading

zcorpan commented Mar 10, 2020

mcking65 commented Mar 17, 2020

smhigley commented Mar 19, 2020

smhigley commented Mar 20, 2020

nschonni left a comment

Choose a reason for hiding this comment

nschonni Mar 20, 2020

Choose a reason for hiding this comment

nschonni Mar 20, 2020

Choose a reason for hiding this comment

smhigley commented Mar 20, 2020

nschonni commented Mar 20, 2020

smhigley commented Apr 3, 2020

zcorpan left a comment • edited Loading

Choose a reason for hiding this comment

zcorpan commented Apr 30, 2020

smhigley commented May 3, 2020

zcorpan left a comment • edited Loading

Choose a reason for hiding this comment

zcorpan left a comment •

edited

Loading

smhigley commented Mar 9, 2020 •

edited

Loading

zcorpan left a comment •

edited

Loading

zcorpan left a comment •

edited

Loading