-
Notifications
You must be signed in to change notification settings - Fork 255
test(cmd-version): add fixture to reproduce issue with empty changelog #1268
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
base: master
Are you sure you want to change the base?
Conversation
tests/fixtures/repos/trunk_based_dev/repo_w_feature_branch_and_release_in_default.py
Show resolved
Hide resolved
5f2547f
to
fa8e3e5
Compare
tests/fixtures/git_repo.py
Outdated
@@ -926,11 +926,11 @@ def simulate_change_commits_n_rtn_changelog_entry( | |||
file_in_repo: str, | |||
) -> SimulateChangeCommitsNReturnChangelogEntryFn: | |||
def _simulate_change_commits_n_rtn_changelog_entry( | |||
git_repo: Repo, commit_msgs: Sequence[CommitDef] | |||
git_repo: Repo, commit_msgs: Sequence[CommitDef], file_path: str | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the change described in the PR title. If I create commits in the feature branch before merging the default branch, it leads to merge conflicts. Making changes in a different file avoids this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually not necessary. The file_in_repo
file's contents actually doesn't matter at all for the use of PSR. We just need a change in a tracked file in order to create commits. We really should just use the --strategy-option
of git merge to forcefully resolve conflicts (the result of the file does not matter).
I just checked the build_repo_from_definition
fixture and unfortunately I did not implement the passing of a strategy option for a GIT_MERGE but I did for GIT_SQUASH. We should just carry that implementation over and then follow what I did in the tests/fixtures/repos/github_flow/repo_w_default_release.py
to pass a strategy_option
value in the repo definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a very good point. Added strategy_option
in this commit and removed file_path
1b8cf1e.
Made it NotRequired
to not update existing places where merge action is used (but I can do it if you think it would be better).
And if you see something that is easy for you to fix and it's blocking a merge - I'm definitely not against you making changes in this PR to move it forward.
81d1e6d
to
d14dc21
Compare
@dzmitrysliaptsou, I just want to say fantastic work. You are the first person to actually implement an E2E test for this project in a long time. Thank you for the effort. I have just a few comments and adjustments and we can get this in. While you have been doing that I have been revamping the release history algorithm to consolidate both it and the version determination algorithm along with a lot of performance caching. This should be a huge improvement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See review comments, if you need help, let me know and I can fix it this weekend and get this merged.
711f9a7
to
eb6dda7
Compare
eb6dda7
to
1b8cf1e
Compare
Purpose
Provides a repo fixture to reproduce the problem described in #1252 (based on this comment)
Rationale
Added a new repo following the examples in other fixtures.
Had to made one more addition:
simulate_change_commits_n_rtn_changelog_entry()
fixture always worked with thefile_in_repo
. For this specific flow it caused merge conflicts, because commit in the feature branch was made before default branch was merged into a feature branch. Made this parameter configurable.How did you test?
I ran the test and it failed with the expected error (empty
## v0.3.0 (2025-05-31)
changelog entry)How to Verify
@pytest.mark.xfail
decorator on thetests/e2e/cmd_version/bump_version/trunk_based_dev/test_repo_w_feature_branch_and_release_in_default.py
test.pytest -vv --comprehensive tests/e2e/cmd_version/bump_version/trunk_based_dev/test_repo_w_feature_branch_and_release_in_default.py
PR Completion Checklist
Reviewed & followed the Contributor Guidelines
Changes Implemented & Validation pipeline succeeds
Commits follow the Conventional Commits standard
and are separated into the proper commit type and scope (recommended order: test, build, feat/fix, docs)
Appropriate Unit tests added/updated
Appropriate End-to-End tests added/updated
Appropriate Documentation added/updated and syntax validated for sphinx build (see Contributor Guidelines)