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

fix(test-version-utils): Fix version calculation to account for legacy breaking minor releases #23603

Merged
merged 8 commits into from
Jan 24, 2025

Conversation

scottn12
Copy link
Contributor

@scottn12 scottn12 commented Jan 17, 2025

This PR fixes N-X version calculation to take into account the new legacy breaking minor releases. For example,

  • For 2.0.0 <= N < 2.10.0, N-1 is 2.0.0-rc.5.0.x.
  • For 2.10.0 <= N < 2.20.0, N-1 is 2.5.x (latest minor between 2.0 and 2.10 is 2.5.x)
  • For 2.20.0 <= N < 2.30.0, N-1 is 2.13.x (latest minor between 2.10 and 2.20 is 2.13.x)

We should still consider a full rewrite of version calculation, since the code is extremely messy and hard to follow for developers not familiar with it already. However, this change is needed to ensure we continue testing properly in the meantime. See AB#8198 for more details.

AB#28437

@github-actions github-actions bot added area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch labels Jan 17, 2025
@scottn12 scottn12 marked this pull request as ready for review January 17, 2025 21:11
@Copilot Copilot bot review requested due to automatic review settings January 17, 2025 21:11

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

@scottn12 scottn12 requested a review from ChumpChief January 23, 2025 16:09
@tylerbutler
Copy link
Member

No objections to this change, really, but I think Sonali wrote some similar code in #22687. Might be worth a look to see if we can consolidate some logic.

@scottn12 scottn12 requested a review from alexvy86 January 23, 2025 19:08
@scottn12
Copy link
Contributor Author

scottn12 commented Jan 23, 2025

No objections to this change, really, but I think Sonali wrote some similar code in #22687. Might be worth a look to see if we can consolidate some logic.

Thanks for bringing this to my attention! I looked into it and unfortunately it does not support going back to internal/RC releases which is required in this scenario. Maybe when we look into long term fixes we can merge the logic

Copy link
Contributor

@alexvy86 alexvy86 left a comment

Choose a reason for hiding this comment

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

Logic looks good as far as I can tell. The unit tests help a lot :). All my comments about the docs, which I think we should address to make them as clear and correct as possible, but approving other than that.

packages/test/test-version-utils/src/versionUtils.ts Outdated Show resolved Hide resolved
packages/test/test-version-utils/src/versionUtils.ts Outdated Show resolved Hide resolved
packages/test/test-version-utils/src/versionUtils.ts Outdated Show resolved Hide resolved
packages/test/test-version-utils/src/versionUtils.ts Outdated Show resolved Hide resolved
packages/test/test-version-utils/src/versionUtils.ts Outdated Show resolved Hide resolved
@scottn12 scottn12 merged commit bc91468 into microsoft:main Jan 24, 2025
31 checks passed
@scottn12 scottn12 deleted the fixN-1Gen branch January 24, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants