Skip to content

Use full schema (schema-base.yaml) in schema.test.mjs - #4702

Merged
ralfhandl merged 2 commits into
OAI:devfrom
handrews:redundant
Jun 13, 2025
Merged

Use full schema (schema-base.yaml) in schema.test.mjs#4702
ralfhandl merged 2 commits into
OAI:devfrom
handrews:redundant

Conversation

@handrews

@handrewshandrews commented Jun 13, 2025

Copy link
Copy Markdown
Member

[NOTE: The schema coverage will fail because the script is not smart enough to figure out that this branch derives from dev.]

This script confused me greatly when trying to fix the coverage, as it looks like the test script that runs. But it is not, the tests run out of the scripts/ directory.

Apparently this is needed, so update it to use the correct schema.

  • schema changes are included in this pull request
  • schema changes are needed for this pull request but not done yet
  • no schema changes are needed for this pull request

@handrews
handrews requested review from a team as code ownersJune 13, 2025 01:33
@handrewshandrews added the script Pull requests that update Bash or JavaScript code label Jun 13, 2025

@ralfhandlralfhandl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This script is not redundant, it validates both "pass" and "fail" test cases and checks whether they pass or fail as expected.

This is needed in addition to the coverage script, which only runs the "pass" test cases.

Not sure if/how we can fold the "fail" test cases into the coverage script.

Copying the defineVocabulary and registerSchema additions from schema-test-coverage.mjs and validating with schema-base instead of schema works fine.

@ralfhandl

Copy link
Copy Markdown
Contributor

This script confused me greatly when trying to fix the coverage, as it looks like the test script that runs. But it is not, the tests run out of the scripts/ directory.

This script is run by vitest and is the "parent" of the test coverage script.

Since we are testing with a placeholder, we need to match
the placeholder. This will unfortunately need to be different
on each new release line branch, so let's separate this test
case into its own file.
@handrewshandrews changed the title Remove apparenlty redundant test script.Use full schema (schema-base.yaml) in schema.test.mjsJun 13, 2025
@handrews

Copy link
Copy Markdown
MemberAuthor

@ralfhandl I have changed this PR to make the same changes as to the other script. It's not idea with the duplication, but please let's get this unblocked first and optimize/refactor later.

@handrews

Copy link
Copy Markdown
MemberAuthor

BTW the commit to fix the jsonSchemaDialect stuff was cherry-picked from v3.1-dev, hopefully git will recognize it is already on that branch, and I can resolve conflicts on the port to v3.2-dev

Comment on lines +29 to +81
addKeyword({
id: "https://spec.openapis.org/oas/schema/vocab/keyword/discriminator",
interpret: (discriminator, instance, context) => {
return true;
},
/* discriminator is not exactly an annotation, but it's not allowed
* to change the validation outcome (hence returing true from interopret())
* and for our purposes of testing, this is sufficient.
*/
annotation: (discriminator) => {
return discriminator;
},
});

addKeyword({
id: "https://spec.openapis.org/oas/schema/vocab/keyword/example",
interpret: (example, instance, context) => {
return true;
},
annotation: (example) => {
return example;
},
});

addKeyword({
id: "https://spec.openapis.org/oas/schema/vocab/keyword/externalDocs",
interpret: (externalDocs, instance, context) => {
return true;
},
annotation: (externalDocs) => {
return externalDocs;
},
});

addKeyword({
id: "https://spec.openapis.org/oas/schema/vocab/keyword/xml",
interpret: (xml, instance, context) => {
return true;
},
annotation: (xml) => {
return xml;
},
});

defineVocabulary(
"https://spec.openapis.org/oas/3.1/vocab/base",
{
"discriminator": "https://spec.openapis.org/oas/schema/vocab/keyword/discriminator",
"example": "https://spec.openapis.org/oas/schema/vocab/keyword/example",
"externalDocs": "https://spec.openapis.org/oas/schema/vocab/keyword/externalDocs",
"xml": "https://spec.openapis.org/oas/schema/vocab/keyword/xml",
},
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You shouldn't need to define the vocabulary or keywords. They're already loaded when you import from @hyperjump/json-schema/openapi-3-1.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jdesrosiers thanks for commenting- the problem is that we're using a schema with a different $id from the published 3.1 schema, as we're testing the in-development branches. Also I was mostly trying to get something else unblocked so I didn't dig as deeply as I might have otherwise.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the problem is that we're using a schema with a different $id from the published 3.1 schema

Right. Registering the meta-schemas immediately after this block addresses that concern. It's just defining the vocabulary (defineVocabulary) and the keywords (addKeyword) that's unnecessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's just defining the vocabulary (defineVocabulary) and the keywords (addKeyword) that's unnecessary.

Apparently we still need defineVocabulary: just registering the meta-schemas results in

Error: Unrecognized vocabulary: https://spec.openapis.org/oas/3.2/vocab/base.
You can define this vocabulary with the 'defineVocabulary' function.

when validating 3.2 OpenAPI descriptions.

I expected that registering meta.yaml - whose title and description claim that it defines the OAS Base Vocabulary - would actually define the vocabulary 😞

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, yeah. I never added support for 3.2. I'll work on it.

@jdesrosiers

Copy link
Copy Markdown
Contributor

Not sure if/how we can fold the "fail" test cases into the coverage script.

There shouldn't be any reason why you can't run the fail tests as well. You're throwing an error when a test fails, but for the purpose of test coverage, we shouldn't care whether the test passes or fails. You could just remove that check and run both sets of tests.

@handrews
handrews deleted the redundant branch June 19, 2025 17:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scriptPull requests that update Bash or JavaScript code

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@handrews@ralfhandl@jdesrosiers