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

Add unit test to investigate and hopefully verify #2454 #2847

Closed
wants to merge 5 commits into from

Conversation

senritsu
Copy link

@senritsu senritsu commented Sep 13, 2021

Description

Added a unit test for investigating branch versioning issues, as prompted by @asbjornu in the discussion of #2454.

Related Issue

#2454

Motivation and Context

The test was initially created to reproduce #2454. The test failed when it was initially created, but passes on current main (with some modifications to the version in the issue discussion). There are a few details that I am still not completely clear on:

  • Should hotfix branches get a beta prerelease tag? In the new test they don't, but according to this line in another test maybe they should?
  • Build metadata does not appear, but that is probably because of the mode CD in the Config used?
  • Some of the versioning in the different tests in the file seemed inconsistent when first writing the test, I did not have the time to fully check that this time around, so it could use some more scrutiny
  • Some comments need to still be removed before the PR should be merged

Disregarding those details, it seems like #2454 is not an issue anymore (?).

How Has This Been Tested?

The change is a unit test, it was executed

Screenshots (if appropriate):

N/A

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@senritsu senritsu changed the title Hotfix versioning issues Add unit test to investigate and hopefully verify #2454 Sep 13, 2021
@asbjornu
Copy link
Member

The tests are successful?

@senritsu
Copy link
Author

senritsu commented Sep 14, 2021

Seems like it, yes. I made a few adjustments because apparently some things changed since the original discussion.

Disregarding the few comments, the test may actually validate that #2454 was unintentionally fixed in the meantime, but I would not hold my breath on it just yet.

@asbjornu
Copy link
Member

The successful test may be a false positive as discovered in #2830 (comment).

@asbjornu
Copy link
Member

Can you please rebase against main to see whether the test is still successful?

@senritsu
Copy link
Author

senritsu commented Oct 4, 2021

I can confirm that the test still passes locally, even with the current changes from main.

I am still unsure if a hotfix branch should have a beta prerelease tag, similar to a release branch. Currently it only has (with versioning mode CD) the same tag like main/master, -ci.n.

When that is clarified I would remove the corresponding comments from the unit test, and the test would seem to confirm that at least one scenario of #2454 appears to work now. Of course that only goes for mode: ContinuousDeployment, and there may be edge cases that are missing.

@senritsu
Copy link
Author

senritsu commented Oct 4, 2021

Curiously, when trying out 5.7.0 with one of our internal library repositories, the hotfix branch does in fact get a beta tag, albeit without mode: ContinuousDeployment. Maybe this warrants another unit test, for the different modes.

This adds another datapoint and confirms that in at least one of our productive scenarios, #2454 now seems to be fixed.

@senritsu
Copy link
Author

senritsu commented Oct 4, 2021

Probably an unrelated issue, but the same package that locally works perfectly fine now still fails to properly account for the hotfix on our buildserver (TeamCity). Locally it versions to 0.5.1, on TC it ignores the hotfix and stays on 0.5.0.

@senritsu
Copy link
Author

senritsu commented Oct 4, 2021

Additional scenarios have been added and both new tests now fail. In both modes the second hotfix is ignored for versioning purposes.

@asbjornu
Copy link
Member

Sorry for not having time to investigate or fix this. If you figure out a solution, feel free to update the PR.

@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. After 30 days from now, it will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 2, 2022
@asbjornu
Copy link
Member

As the accompanying issue #2454 is closed with a workaround, is this PR relevant anymore?

@arturcic
Copy link
Member

Closes in favor of #3445

@arturcic arturcic closed this Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants