Skip to content

add tests for symbol levels#13305

Merged
feerrenrut merged 12 commits into
masterfrom
addMoveByWordSymLevelTests
Feb 16, 2022
Merged

add tests for symbol levels#13305
feerrenrut merged 12 commits into
masterfrom
addMoveByWordSymLevelTests

Conversation

@feerrenrut

@feerrenrut feerrenrut commented Feb 3, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Link to issue number:

Related to issue #11779, #10855, and PR #12710

Summary of the issue:

PR #12710 has been suffering from scope creep, several of the problems discussed pre-existed in NVDA.

With Symbol level all:

  • '👕' symbol ("t-shirt") spoken as 't dash shirt' when moving by word or character
  • Text "don't" spoken as "don tick t" when moving by word
  • Symbols in NVDA speech UI (EG "spelling error" in French "Faute d'orthographe") are processed as symbols. The example "Faute d'orthographe" becomes "Faute d apostrophe orthographe", equivalent to "spelling e tick rror" in English, except that "apostrophe" is a longer word than "tick".

Description of how this pull request fixes the issue:

Note, this PR does not introduce user visible changes. It is intended to help developers reason about the behavior of NVDA.

  • Test (to demonstrate) behavior with Symbol level All and None (as the boundary cases for behavior) when moving by word, line, character.
  • Test (to demonstrate) behavior with Symbol Level All when speech UI contains a symbol. For this, the System Test Spy global plugin had to be modified to alter translation strings of NVDA at runtime.

Testing strategy:

System tests.

Known issues with pull request:

None

Change log entries:

None

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@feerrenrut
Add tests for move by line and char
7ead465 
"symbol level word" tests are disabled, expectations for these tests
match the default symbol level tests to make change in behavior more
obvious.
@feerrenrut
remove unused tests, test symbol level all and none
77959ee
@feerrenrut
Consistant sym level None, all for tests
a7af3c9
@feerrenrut
test symbol substitution in Speech UI
d8fada4
@feerrenrut feerrenrut marked this pull request as ready for review February 3, 2022 08:19
@feerrenrut feerrenrut requested a review from a team as a code owner February 3, 2022 08:19
@feerrenrut feerrenrut requested a review from seanbudd February 3, 2022 08:19
@feerrenrut feerrenrut mentioned this pull request Feb 3, 2022
Closed
8 tasks
lukaszgo1
lukaszgo1 reviewed Feb 3, 2022
View reviewed changes
Comment thread tests/system/robot/notepadTests.py Outdated
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@feerrenrut

feerrenrut commented Feb 8, 2022

Copy link
Copy Markdown
Contributor Author

We may want to add a test for symbols in table headers, see #12710 (comment)

@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@feerrenrut

feerrenrut commented Feb 8, 2022

Copy link
Copy Markdown
Contributor Author

We may also want to add tests for selecting text by word, see #11779 (comment)

seanbudd
seanbudd previously approved these changes Feb 8, 2022
View reviewed changes

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, do we have issues for the other test cases you want to cover? Or should they go in this PR?

@feerrenrut

feerrenrut commented Feb 9, 2022

Copy link
Copy Markdown
Contributor Author

After looking over #11779 and #10855 again, I think we should extend the move by word test to explicitly demonstrate multiple symbols combined, ie two bar symbols: ||

I'll also look at adding tests for the selection and table cases. The table case will need to be a Chrome test.

@feerrenrut

feerrenrut commented Feb 11, 2022

Copy link
Copy Markdown
Contributor Author

I have added the extra test scenarios.

@feerrenrut feerrenrut requested a review from seanbudd February 11, 2022 01:19
seanbudd
seanbudd reviewed Feb 11, 2022
View reviewed changes

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM other than the following addition. Due to the size of the PR, I plan to give it another read over just to see if anything else needs clarification.

Comment thread tests/system/robot/symbolPronunciationTests.py Outdated
@seanbudd seanbudd dismissed their stale review February 11, 2022 04:40

remnant approval from stale review

@feerrenrut feerrenrut requested a review from seanbudd February 11, 2022 09:28
lukaszgo1
lukaszgo1 reviewed Feb 11, 2022
View reviewed changes
Comment thread tests/system/robot/symbolPronunciationTests.py Outdated
@feerrenrut
Use several varied symbols as a word
dd26001 
Demonstrate another silient word case, where multiple symbols make up
a word (in notepad.exe). Expected behaviour unclear.
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
seanbudd
seanbudd approved these changes Feb 16, 2022
View reviewed changes

@seanbudd seanbudd left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good

@feerrenrut feerrenrut merged commit b0be1c2 into master Feb 16, 2022
@feerrenrut feerrenrut deleted the addMoveByWordSymLevelTests branch February 16, 2022 03:40
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@seanbudd seanbudd seanbudd approved these changes

+1 more reviewer

@lukaszgo1 lukaszgo1 lukaszgo1 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2022.1

Development

Successfully merging this pull request may close these issues.

5 participants

@feerrenrut @AppVeyorBot @seanbudd @lukaszgo1 @nvaccessAuto