Skip to content

fix(web): use actual legacy model for legacy-model support unit tests#14460

Merged
jahorton merged 1 commit intomasterfrom
fix/web/legacy-model-test
Aug 6, 2025
Merged

fix(web): use actual legacy model for legacy-model support unit tests#14460
jahorton merged 1 commit intomasterfrom
fix/web/legacy-model-test

Conversation

@jahorton
Copy link
Copy Markdown
Contributor

@jahorton jahorton commented Aug 5, 2025

Noticed this during work on epic-autocorrect when the "legacy model" tests failed from intermediate changes that shouldn't - and, in fact, didn't - apply to legacy models.

The TrieModel is non-legacy - it's very much updated for 14.0+ features. The old DummyModel, though... that one does not leverage 14.0+ features, thus can test those pathways. Sadly, it doesn't use keying - it just "dummies in" specific suggestions, so the corresponding legacy test (for use of keying) is now skipped.

@keymanapp-test-bot skip

@darcywong00
Copy link
Copy Markdown
Contributor

Some nits about the PR title/body 😄

  1. The title should start with an action word (instead of legacy model)
  2. I see this ends up disabling the should compose suggestions from a fat-fingered keypress (keying needed) test because there's no suitable model for the test. Maybe we note this in the PR body.

Copy link
Copy Markdown
Contributor

@darcywong00 darcywong00 left a comment

Choose a reason for hiding this comment

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

lgtm

@jahorton jahorton changed the title fix(web): legacy model unit tests should use an actual legacy model fix(web): use actual legacy model for legacy-model support unit tests Aug 6, 2025
@jahorton jahorton merged commit a5bd24b into master Aug 6, 2025
19 checks passed
@jahorton jahorton deleted the fix/web/legacy-model-test branch August 6, 2025 13:39
@github-project-automation github-project-automation bot moved this from Todo to Done in Keyman Aug 6, 2025
@jahorton
Copy link
Copy Markdown
Contributor Author

jahorton commented Aug 8, 2025

  1. I see this ends up disabling the should compose suggestions from a fat-fingered keypress (keying needed) test because there's no suitable model for the test. Maybe we note this in the PR body.

Will there ever be a suitable model? If not, shouldn't we just delete the test?

Thought about deleting, but it is a possible case. Not one we've been made aware of, of course - at this point, it'd only matter if someone out there actually did write and distribute their own custom model.

@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Aug 9, 2025

Thought about deleting, but it is a possible case. Not one we've been made aware of, of course - at this point, it'd only matter if someone out there actually did write and distribute their own custom model.

I think then that we should delete it. We can always put it back if the need arises. Having skipped tests is just an unnecessary flag in test results otherwise.

@keyman-server
Copy link
Copy Markdown
Collaborator

Changes in this pull request will be available for download in Keyman version 19.0.95-alpha

@keymanapp keymanapp deleted a comment from keyman-server Aug 13, 2025
@keymanapp keymanapp deleted a comment from mcdurdin Aug 13, 2025
@keymanapp keymanapp deleted a comment from keyman-server Aug 13, 2025
@keymanapp keymanapp deleted a comment from keyman-server Aug 13, 2025
@keymanapp keymanapp deleted a comment from keyman-server Aug 13, 2025
@keymanapp keymanapp deleted a comment from keyman-server Aug 13, 2025
@keymanapp keymanapp deleted a comment from keyman-server Aug 13, 2025
@keymanapp keymanapp deleted a comment from keyman-server Aug 13, 2025
@keyman-server
Copy link
Copy Markdown
Collaborator

Changes in this pull request will be available for download in Keyman version 19.0.95-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants