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

Create updated tests for APG design pattern example: checkbox #447

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Jun 10, 2021

@zcorpan zcorpan force-pushed the bocoup/update-checkbox-tests branch 3 times, most recently from e7a7bb6 to 996b459 Compare June 24, 2021 12:24
@zcorpan
Copy link
Member Author

zcorpan commented Jun 24, 2021

For some reason npm run create-all-tests doesn't create the tests for checkbox. I've probably made a mistake somewhere, but there's no useful error message. I haven't yet figured out what's wrong.

@zcorpan zcorpan force-pushed the bocoup/update-checkbox-tests branch 3 times, most recently from 5ccbb81 to 05723e1 Compare July 2, 2021 12:35
@zcorpan zcorpan marked this pull request as ready for review July 2, 2021 12:41
@zcorpan
Copy link
Member Author

zcorpan commented Jul 2, 2021

OK, I think everything works now! Ready for review.

@jscholes
Copy link
Contributor

jscholes commented Jul 2, 2021

@zcorpan

Example

  1. The reference directory contains assets relating to the tri-state checkbox (checkbox2) as well as the one under test.
  2. The default JS/CSS references haven't been removed from the example (e.g. the W3C stylesheet, references to JS assets from the APG repo that we don't use).
  3. The example has been modified to the degree that the heading structure now skips levels (<h1> followed by <h3>). Usually we don't cut this much content out.

For more info, see #394. Some of these issues seem to have already been present in the checkbox plan, but we should probably get them fixed up if we're submitting updates to it.

commands.csv

The JAWS commands still include smart navigation commands which are no longer in scope.

tests.csv

  1. The navigation tests don't include any state in their names. E.g. test 1 and test 9 have the same title ("Navigate forwards to checkbox in reading mode").
  2. The inclusion of an article in test titles is inconsistent. E.g. "Navigate forwards to checkbox" versus "Operate a checkbox" (no "a" in the first one).
  3. The "operate a checkbox" tests aren't separated based on the state transition. I.e. we need tests to check a checkbox, and separate ones to uncheck it.

I have some additional thoughts about the group-related tests, but I'll revisit those.

Test Preview

The index page is working, but the review page for checkbox seems completely broken. For example I see:

  • Test 1: Navigate forwards to checkbox in reading mode
  • Test 2: Navigate to an unchecked checkbox in reading mode

The second of those isn't even present in tests.csv? So I have no idea where it's coming from. The same applies all the way down, with old tests interleaved with the new.

@zcorpan zcorpan force-pushed the bocoup/update-checkbox-tests branch from 1965f2b to 4538cc3 Compare August 26, 2021 13:38
@zcorpan zcorpan force-pushed the bocoup/update-checkbox-tests branch from 4cdab95 to b2e2083 Compare August 26, 2021 13:57
@zcorpan
Copy link
Member Author

zcorpan commented Aug 26, 2021

@zcorpan

Example

  1. The reference directory contains assets relating to the tri-state checkbox (checkbox2) as well as the one under test.
  2. The default JS/CSS references haven't been removed from the example (e.g. the W3C stylesheet, references to JS assets from the APG repo that we don't use).
  3. The example has been modified to the degree that the heading structure now skips levels (<h1> followed by <h3>). Usually we don't cut this much content out.

For more info, see #394. Some of these issues seem to have already been present in the checkbox plan, but we should probably get them fixed up if we're submitting updates to it.

Thanks, fixed.

commands.csv

The JAWS commands still include smart navigation commands which are no longer in scope.

OK. How should those lines be changed?

(I haven't yet addressed the remaining.)

@jscholes
Copy link
Contributor

@zcorpan

OK. How should those lines be changed?

Just remove any command from all rows that includes smart navigation.

@zcorpan
Copy link
Member Author

zcorpan commented Dec 23, 2021

I've addressed the comments on the source files.

I don't know what was up with the test preview.

@jscholes

I have some additional thoughts about the group-related tests, but I'll revisit those.

What about them? :)

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.

2 participants