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

Test version command #12576

Merged
merged 1 commit into from
Feb 26, 2025
Merged

Test version command #12576

merged 1 commit into from
Feb 26, 2025

Conversation

maxproske
Copy link
Contributor

What I did

Added unit tests for docker compose version to ensure correct formatting.

@maxproske maxproske requested a review from a team as a code owner February 24, 2025 05:36
@maxproske maxproske requested review from ndeloof and glours February 24, 2025 05:36
@ndeloof
Copy link
Contributor

ndeloof commented Feb 25, 2025

I honestly don't feel like this command requires a test...

@maxproske
Copy link
Contributor Author

maxproske commented Feb 25, 2025

@ndeloof That's fair, I added the test because:

  1. I'm starting small to learn the codebase
  2. It's often the first command users run to verify their installation
  3. Each format has required a fix which this guards against Fix json format for version command #9242 Return only numbers in short version #9096

Please let me know what areas you'd prefer to prioritize for future tests.

Copy link
Contributor

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

LGTM with a minor note

defer func() {
internal.Version = originalVersion
}()
internal.Version = "v2.33.0-desktop.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

This version string is very specific to Docker Desktop and will be outdated once we release v2.34, I fear this will bring some confusion. Better use a pure fake version for testing purpose: v9.9.9-test for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ndeloof Good suggestion, done. If there are other areas you'd like unit tests prioritized, just shout.

@maxproske maxproske force-pushed the feature/test-version branch from c32f122 to 6a993dc Compare February 26, 2025 14:45
@ndeloof ndeloof enabled auto-merge (rebase) February 26, 2025 15:08
Signed-off-by: Max Proske <[email protected]>
auto-merge was automatically disabled February 26, 2025 15:16

Head branch was pushed to by a user without write access

@maxproske maxproske force-pushed the feature/test-version branch from 6a993dc to 3fbe1f8 Compare February 26, 2025 15:16
@glours glours enabled auto-merge (rebase) February 26, 2025 15:25
@glours glours merged commit 876ecc4 into docker:main Feb 26, 2025
26 checks passed
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.

3 participants