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

Validate WritingSystemIds on creation #1230

Merged
merged 13 commits into from
Nov 15, 2024

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Nov 12, 2024

Fix #984.

We now validate WritingSystemId records on creation and throw ArgumentException for any invalid writing system IDs. The special ID value "default" is allowed, all other writing system IDs must be considered valid by the SIL.WritingSystems validation logic.

Special ID "default" is allowed, all other writing system IDs must be
considered valid by the SIL.WritingSystems validation logic.
@rmunn rmunn requested a review from hahn-kev November 12, 2024 07:18
@rmunn rmunn self-assigned this Nov 12, 2024
@rmunn
Copy link
Contributor Author

rmunn commented Nov 12, 2024

Five failing unit tests when I run this locally:

  • LcmCrdt.Tests.EntityCopyMethodTests.EntityCopyMethodShouldCopyAllFields - Invalid writing system ID "Web"
  • LcmCrdt.Tests.DataModelSnapshotTests.VerifyIObjectWithIdsMatchAdapterGetObjectTypeName - Invalid writing system ID "Group"
  • FwLiteProjectSync.Tests.UpdateDiffTests.ExampleSentenceDiffShouldUpdateAllFields - Invalid writing system ID "gz"
  • FwLiteProjectSync.Tests.UpdateDiffTests.SenseDiffShouldUpdateAllFields - Invalid writing system ID "wb"
  • FwLiteProjectSync.Tests.UpdateDiffTests.EntryDiffShouldUpdateAllFields - Invalid writing system ID "ez"

The three FwLiteProjectSync tests are failing because AutoFaker.Generate() is generating randomly-named writing system IDs, and we're no longer allowing that. We'll need to teach Generate() to pick writing system IDs randomly from an allowable list.

... And the two LcmCrdt tests are failing for the same reason: there's an AutoFaker.Generate() call in there, which needs to be taught how to pick valid writing system IDs.

Update: Same five tests are failing in GHA, for the same reason.

Now when AutoFaker.Generate creates a random writing system ID, it will
be chosen from the list of all valid 2-letter writing system IDs.
@rmunn
Copy link
Contributor Author

rmunn commented Nov 12, 2024

Commit 30bdc0f fixes the three FwLiteProjectSync tests. But interestingly, the two failing LcmCrdt tests are still generating writing systems that are randomly-chosen English words, rather than two-letter codes. Something in those tests isn't calling AutoFaker.Generate correctly...

@rmunn
Copy link
Contributor Author

rmunn commented Nov 12, 2024

Commit 5d94a57 passes the MultiStringOverride to the two tests that were failing — but it's not being called correctly. So far I'm not sure why.

@rmunn rmunn removed the request for review from hahn-kev November 12, 2024 07:50
@rmunn
Copy link
Contributor Author

rmunn commented Nov 12, 2024

Kevin has asked for some explicit unit tests around writing system ID validation, especially making sure that custom audio writing systems are going to validate. So I'm removing the review request, and I'll bring it back when those tests are written.

Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

looks good, I left a comment about reusing the language list from SIL.WritingSystems instead of hardcoding our own

backend/FwLite/MiniLcm.Tests/WritingSystemCodes.cs Outdated Show resolved Hide resolved
@hahn-kev hahn-kev self-assigned this Nov 13, 2024
rmunn and others added 6 commits November 14, 2024 10:00
Still some test failures, where AutoFaker.Generate is apparently
generating strings (and therefore not using the WritingSystemIdOverride
class), but those strings are actually writing system IDs and should
have been chosen from the valid list.
# Conflicts:
#	backend/FwLite/FwLiteProjectSync.Tests/Fixtures/MultiStringOverride.cs
#	backend/FwLite/FwLiteProjectSync.Tests/UpdateDiffTests.cs
#	backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs
#	backend/FwLite/MiniLcm.Tests/FakerOverrids/MultiStringOverride.cs
@rmunn rmunn merged commit a5309ad into develop Nov 15, 2024
11 checks passed
@rmunn rmunn deleted the feat/validate-writing-system-ids-in-minilcm branch November 15, 2024 03:35
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.

[MiniLcm] integrate SIL.WritingSystems to validate WritingSystem Ids
2 participants