Skip to content

Schema tests should be only for latest version#1857

Merged
dgageot merged 1 commit intodocker:mainfrom
dgageot:schematest
Feb 26, 2026
Merged

Schema tests should be only for latest version#1857
dgageot merged 1 commit intodocker:mainfrom
dgageot:schematest

Conversation

@dgageot
Copy link
Member

@dgageot dgageot commented Feb 26, 2026

No description provided.

@dgageot dgageot requested a review from a team as a code owner February 26, 2026 10:44
Copy link

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

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

Review Summary

✅ This PR successfully consolidates schema tests to focus only on the latest version. The refactoring is well-executed:

  • Removes duplicate schema tests from v4 and v5 packages
  • Consolidates TestJsonSchemaWorksForExamples into the main schema test file
  • Properly updates import paths to reference latest types
  • Correctly adjusts the schema file path for the new location

Minor observation: There's variable shadowing in TestJsonSchemaWorksForExamples where schemaFile (the local variable containing file contents) shadows the schemaFile constant (the file path). While this is valid Go code and works correctly, consider using a different variable name like schemaBytes or schemaData for clarity.

The changes achieve the stated goal without introducing functional issues.

rumpl
rumpl previously approved these changes Feb 26, 2026
Copy link
Contributor

@doringeman doringeman left a comment

Choose a reason for hiding this comment

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

Thanks! Cherry-picked this on top of #1847 and it fixes task test. 🙌

Signed-off-by: David Gageot <david.gageot@docker.com>
@dgageot dgageot merged commit 76a05f0 into docker:main Feb 26, 2026
5 checks passed
@dgageot dgageot deleted the schematest branch February 27, 2026 19:38
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.

4 participants