Repository navigation
test: Make every invalid fixture fail for the reason it is named for - #1432
Open
isaacschepp wants to merge 5 commits into
Open
isaacschepp wants to merge 5 commits into
isaacschepp wants to merge 5 commits into
Conversation
Most testdata/invalid archives had drifted as the schema changed and failed
validation for unrelated reasons, so they guarded nothing:
- The four assertion-participant fixtures gave subject as a bare string (the
schema wants a typed reference such as {event: ...}), their event had no
participants, and they defined no vocabularies. invalid-person and
invalid-role never reached the check they are named for.
- broken-references, comprehensive-broken-references and
missing-required-fields used fields that now live under properties
(locator, text_from_source, description), dropped fields (tags), a removed
value_type: reference, and a stale source type, so validation stopped at the
schema stage before any reference was checked.
- invalid-properties, invalid-relationship-participants and
invalid-assertion-properties failed only on vocabularies they never defined.
Each fixture now defines what it uses and fails only for its stated error.
TestInvalidFixtures validates every directory under testdata/invalid, checks
the errors listed for it, and fails on any other reported error, or on a
directory with no entry. Putting the old invalid-person fixture back makes it
fail with "unexpected error, so the fixture fails for another reason".
Three fixtures never failed and are not invalid, so they move to
testdata/valid: assertion-unknown-property (meant to pass, with a warning that
needs #1224), invalid-properties as unknown-person-properties
(unknown person properties are warnings by design) and missing-vocabularies
(the standard vocabularies fill in). TestValidFixturesWithWarnings checks
them. invalid-assertion-properties stays, skipped until #1224
lands.
The testdata README now lists every fixture, says which test checks it, and
notes that validating a single file skips cross-reference checks.
Signed-off-by: Isaac Schepp <isaac.schepp@gmail.com>
Deploying genealogix with
|
| Latest commit: |
9dc6abd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ebc5e3d3.genealogix.pages.dev |
| Branch Preview URL: | https://test-invalid-fixtures.genealogix.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The fixture was a near-copy of the complete-family example: about 30 files of persons, places, events, relationships, repositories and vocabularies, around one assertion with three broken references. All of that filler was valid, read by no test, and had to follow every schema change. It did not: it is what made the fixture fail on removed fields instead of its references. It now holds only the assertion, the citation and source the assertion validly references, and the confidence vocabulary it uses. Validation reports exactly the three broken references, as TestInvalidFixtures requires. Removing the events also removes the review findings on event-births.glx: the non-canonical subject role, the trailing whitespace and the descriptions that restated the event. Signed-off-by: Isaac Schepp <isaac.schepp@gmail.com>
source-census.glx, citation-census.glx and assertion-john-birth.glx held source-census-1851, citation-census-john and assertion-john-birth-date. Each file now carries its entity's ID, the layout glx writes and the complete-family example uses. Signed-off-by: Isaac Schepp <isaac.schepp@gmail.com>
citation-birth exists to test its two broken references. The locator was unrelated to them, had already broken the fixture once, and relied on a citation_properties vocabulary the fixture does not ship. The trailing blank line goes with it. Signed-off-by: Isaac Schepp <isaac.schepp@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
A review of
glx/testdata/invalid/assertion-participant-invalid-person/archive.glxfound that it fails validation, but never for the broken person reference it is named for. No test loads it, so nothing noticed. Validating every invalid fixture showed most of them had drifted the same way as the schema changed. They "failed", but only on fields the schema dropped or vocabularies they never defined, so they guarded nothing.assertion-participant-invalid-personsubjectas a bare string, event withoutparticipantsPerson references non-existent persons: person-nonexistentonlyassertion-participant-invalid-roleRole references non-existent participant_roles: invalid-roleonlyassertion-participant-and-valuesubject, plus its ruleassertion-participant-and-propertyparticipantsbroken-referenceslocator,text_from_source,description,tags; missing roles and source typescomprehensive-broken-referenceslocator;value_type: referencemissing-required-fieldslocatorand second missing fieldsinvalid-relationship-participantsThe fixes move each stale field to its current home (
properties.locator,properties.description, …), give the assertion fixtures a typedsubjectand event participants, and give each fixture the vocabulary entries it uses. Directory fixtures get copies of the standard files, checked byTestExampleVocabulariesMatchStandard. Single-file fixtures get only the inline entries they need. That way no fixture depends on how #1355 (directory archives not falling back to the standard vocabularies) is resolved.A second commit cuts
broken-referencesdown from about 30 files, a near-copy of the complete-family example, to the one assertion it tests: the citation and source it validly references, and the confidence vocabulary it uses. All the filler was valid, read by no test, and had to follow every schema change; it is what made the fixture fail on removed fields in the first place.A test that keeps them honest
TestInvalidFixtures(glx/testdata_fixtures_test.go) validates every directory undertestdata/invalidas a directory, which runs the cross-reference stage as well. It then checks:invalidFixturesis reported;Putting the old
assertion-participant-invalid-personback makes it fail withunexpected error, so the fixture fails for another reason: - /assertions/assertion-invalid/subject: 'oneOf' failed.Fixtures that were never invalid
Three never failed, and should not. They move to
testdata/valid/, andTestValidFixturesWithWarningschecks them:assertion-unknown-property: its own header says it must pass, with a warning. It was failing only on an undefinedmediumconfidence. It now passes, but it cannot warn yet, because the validator does not check assertion property names (Validator never checks assertionpropertyagainst property vocabularies, despite the spec saying it does #1224).invalid-properties→unknown-person-properties: unknown person properties are warnings by design. It now passes with exactly its two warnings.missing-vocabularies: the standard vocabularies fill in, so a person with standard properties is valid with no warning. The README had claimed it should warn.invalid-assertion-propertiesstays ininvalid/, skipped with a pointer to #1224. With its missing citation added, it will fail for the right reason once that lands.Related issues
Refs #1224 and #1355. Neither is closed by this PR.
Review focus
invalidFixtures. The two mutual-exclusion fixtures can only be asserted as'not' failed, because the schema validator does not name the rule. I confirmed that each passes once its conflicting field is removed.valid/, one of them under a new name.Testing
go test -count=1 ./glx/...passes (both packages). 13 invalid fixtures pass and 1 is skipped; 3 valid-with-warnings cases pass.golangci-lint run ./glx/...reports nothing for the new test file.Breaking changes
None. Tests and test data only, so no CHANGELOG entry.