Skip to content

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dzmitrysliaptsou
Copy link

@dzmitrysliaptsou dzmitrysliaptsou commented May 31, 2025

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 the file_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)

>           assert expected_md_changelog_content == actual_md_changelog_content
E           assert == failed. [pytest-clarity diff shown]
E             
E             LHS vs RHS shown below
E             
E               # CHANGELOG
E               
E               <!-- version list -->
E               
E               ## v0.3.0 (2025-05-31)
E             + 
E             + ### Features
E             + 
E             + - Add new feature in the feature branch
E             +   
E             ([`0000000`](https://example.com/example_owner/example_repo/commit/0000000000000
E             000000000000000000000000000))
E               
E               
E               ## v0.2.0 (2025-05-31)
E               
E               ### Features
E               
E               - Add another feature
E                 ([`0000000`](https://example.com/example_owner/example_repo/commit/000000000
E             0000000000000000000000000000000))
E               
E               
E               ## v0.1.0 (2025-05-31)
E               
E               - Initial Release
E             

How to Verify

  1. Comment (or remove @pytest.mark.xfail decorator on the tests/e2e/cmd_version/bump_version/trunk_based_dev/test_repo_w_feature_branch_and_release_in_default.py test.
  2. run 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)

@@ -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
Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Author

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.

@dzmitrysliaptsou dzmitrysliaptsou force-pushed the test/add-fixtures-merge-to-feature-after-release branch 2 times, most recently from 81d1e6d to d14dc21 Compare May 31, 2025 20:32
@codejedi365
Copy link
Contributor

@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.

Copy link
Contributor

@codejedi365 codejedi365 left a 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.

@dzmitrysliaptsou dzmitrysliaptsou changed the title test(cmd-version): Add fixture to reproduce issue with empty changelog test(cmd-version): add fixture to reproduce issue with empty changelog Jun 6, 2025
@dzmitrysliaptsou dzmitrysliaptsou force-pushed the test/add-fixtures-merge-to-feature-after-release branch 2 times, most recently from 711f9a7 to eb6dda7 Compare June 6, 2025 07:35
@dzmitrysliaptsou dzmitrysliaptsou force-pushed the test/add-fixtures-merge-to-feature-after-release branch from eb6dda7 to 1b8cf1e Compare June 6, 2025 07:36
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.

2 participants