Skip to content

feat(web): Gesture Recognizer unit testing 🐵#6970

Merged
jahorton merged 37 commits intofeat/web/gesture-recognizer-mainfrom
feat/web/recognizer-input-unit-testing-2
Aug 16, 2022
Merged

feat(web): Gesture Recognizer unit testing 🐵#6970
jahorton merged 37 commits intofeat/web/gesture-recognizer-mainfrom
feat/web/recognizer-input-unit-testing-2

Conversation

@jahorton
Copy link
Copy Markdown
Contributor

@jahorton jahorton commented Jul 19, 2022

Continuing from the work seen in #6952, this PR establishes the unit-testing toolchain (karma, mocha, chai, etc) needed for gesture-recognizer unit tests to ensure robustness down the road.

After looking at the possible approaches we can take for unit testing the module, to ensure good code coverage, I believe we'll need two separate layers for unit tests.

  1. DOM events -> input coordinate sequences
    • Ensuring that a properly-configured gesture receiver interprets events exactly as we expect and can reproduce previously-recorded sequences.
    • Timestamp emulation isn't perfect, though it's possible to approximate.
  2. Input coordinate sequences -> gestures
    • By starting from input coordinate sequences, we can bypass the DOM entirely and use headless mode here.
    • We'll take valid timestamps on recorded sequences for granted here.

Timestamp emulation is possible due to a feature in https://github.com/sinonjs/fake-timers we haven't been using before (as found within SinonJS). I did decide to update our dev-dependency version for the containing package, as it was missing a rather helpful method for implementing the desired unit tests - runAllAsync wasn't supported in the old version.

At this point, we can only address point 1 above; there is no gesture segmentation or synthesis code written yet.


One big question worth asking:

  • Would we be fine running unit tests without BrowserStack?
    • This should be possible if we run against just Firefox and Chrome, as both are generally supported on all major platforms.
    • May require a bit of build-agent setup (if the agents lack either browser at present).

Unlike with Keyman Engine for Web, we don't need to worry much about browser-specific keymappings here, let alone use of touch-alias elements, rotation, or the like. I could see worry about the shifting layout of the keyboard that results from rotation, but fortunately, the design addresses that almost inherently... and there's little issue with writing a unit test or two to ensure that shifts in important layout / configuration elements don't cause issues.

For now, I've provided a BrowserStack-based configuration (for CI) for the browser-based unit tests, but in my opinion, we should be safe to forego use of their service for this module if we could guarantee consistently-available browser(s) for use on all our build agents. ... which is probably why we're using BrowserStack, true. I don't think we need as extensive browser-coverage here, at the least.


@keymanapp-test-bot skip

All the previously-defined user tests are now unit tests!

@jahorton jahorton added this to the A16S6 milestone Jul 19, 2022
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Jul 19, 2022
@keymanapp-test-bot
Copy link
Copy Markdown

keymanapp-test-bot bot commented Jul 19, 2022

@jahorton jahorton force-pushed the feat/web/recognizer-input-unit-testing-2 branch from ea5331e to 3763f3f Compare July 19, 2022 03:51
@jahorton
Copy link
Copy Markdown
Contributor Author

Alright, finally operational.

image

Though... maybe I should specify slightly more up-to-date targets for the tests; looks like the old Firefox version doesn't have .finally() support for Promises!

Comment on lines +53 to +61
'desktopRoamAndReturn',
'mobileSafeZoneCancel',
'mobileProximityApproach',
'embeddedBorderCancel',
'hardBorderCancel',
'popupLongRoamingEnd',
'popupShimCancel',
'popupSafePersistence',
'basicMultitouch'
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Each of these corresponds to a user test defined on #6878; they're listed in order of their definitions on that PR.

I opted to give them more meaningful/descriptive names as unit tests.

Note that 'popupSafePersistence' (#6878's TEST_4_3_2_SAFE_ZONE) is slightly tweaked from its original version on that PR, as its written form there was a little different from the intended test.

@@ -0,0 +1,163 @@
{
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that it is ill-advisable to spend much time reviewing the recorded-sequence .json files found in this folder.

I've pretty-printed them so that they may be easily read and edited manually if necessary... but all of this is largely auto-generated. These files account for all but about 850 lines of changes at the time of this comment.

@jahorton jahorton marked this pull request as ready for review July 27, 2022 05:50
Base automatically changed from feat/web/recognizer-input-unit-testing to feat/web/gesture-recognizer-main August 2, 2022 07:50
@mcdurdin mcdurdin modified the milestones: A16S7, A16S8 Aug 7, 2022
@jahorton jahorton dismissed mcdurdin’s stale review August 8, 2022 04:02

Major updates to base branch, including dependency update

Copy link
Copy Markdown
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

LGTM

@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Aug 9, 2022

One big question worth asking:

  • Would we be fine running unit tests without BrowserStack?
    This should be possible if we run against just Firefox and Chrome, as both are generally supported on all major platforms.
    May require a bit of build-agent setup (if the agents lack either browser at present).

IMO, I think we want to be testing against stable, known versions. Currently we don't maintain the browser versions on build agents and probably best we don't start now. Also in my experience browsers don't always shut down correctly and that would create a maintenance burden on the agents.

Though... maybe I should specify slightly more up-to-date targets for the tests; looks like the old Firefox version doesn't have .finally() support for Promises!

This is only for tests, right, not for KeymanWeb itself?

@jahorton
Copy link
Copy Markdown
Contributor Author

Though... maybe I should specify slightly more up-to-date targets for the tests; looks like the old Firefox version doesn't have .finally() support for Promises!

This is only for tests, right, not for KeymanWeb itself?

It's only tests that are using Promise.finally(), yes.

@jahorton jahorton added epic A long lived branch, home for a new feature, usually will have child PRs based on it and removed epic A long lived branch, home for a new feature, usually will have child PRs based on it labels Aug 11, 2022
@jahorton jahorton merged commit ff76082 into feat/web/gesture-recognizer-main Aug 16, 2022
@jahorton jahorton deleted the feat/web/recognizer-input-unit-testing-2 branch August 16, 2022 03:20
@jahorton jahorton mentioned this pull request Sep 19, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants