Skip to content

Fix error when adding files to existing PR with --update-pr and refactor + fixes in options tests#4962

Merged
boegel merged 13 commits intoeasybuilders:developfrom
Flamefire:20250723180922_new_pr_wXUzyZFgvE
Aug 1, 2025
Merged

Fix error when adding files to existing PR with --update-pr and refactor + fixes in options tests#4962
boegel merged 13 commits intoeasybuilders:developfrom
Flamefire:20250723180922_new_pr_wXUzyZFgvE

Conversation

@Flamefire
Copy link
Copy Markdown
Contributor

@Flamefire Flamefire commented Jul 23, 2025 •
edited by boegel
Loading

(created using eb --new-pr)

When only adding files to an existing PR (e.g. missing dependencies) the first check requires a commit message and this one requires NO commit message unless --force is given.

Remove the requirement when there is a source-branch (i.e. update-pr and update-branch) (in first commit)
I added tests for verifying the behavior is still correct (2nd and 3rd commit).

During that I noticed some issues in the tests which are present throughout the file so I did a (mostly search&replace) refactoring for the asserts using regexs

By that I noticed a test that didn't really test much: Matching the list of easyblocks, which contains |-- bar in the regex means that ANY match (e.g. --bar) is sufficient due to the missing escaping. But we wanted to test the full text for which we don't even need a regex but a simple assertIn
See the last commit which adds 23(!!!) missing lines

edit: fixes #4963

@Flamefire Flamefire force-pushed the 20250723180922_new_pr_wXUzyZFgvE branch from e9b8739 to bf90425 Compare July 23, 2025 16:10
@Flamefire Flamefire force-pushed the 20250723180922_new_pr_wXUzyZFgvE branch from bf90425 to 973aaec Compare July 25, 2025 09:10
@Flamefire Flamefire changed the title Fix error when adding files to existing PR Fix error when adding files to existing PR and refactor test file Jul 25, 2025
@Flamefire Flamefire force-pushed the 20250723180922_new_pr_wXUzyZFgvE branch 2 times, most recently from 329eba1 to da20a23 Compare July 25, 2025 13:02
@Flamefire Flamefire force-pushed the 20250723180922_new_pr_wXUzyZFgvE branch from da20a23 to bde2f1a Compare July 25, 2025 13:22
@boegel boegel added the bug fix label Jul 30, 2025
@boegel boegel added this to the release after 5.1.1 milestone Jul 30, 2025
@branfosj branfosj mentioned this pull request Jul 31, 2025
Closed
@boegel boegel changed the title Fix error when adding files to existing PR and refactor test file Fix error when adding files to existing PR and refactor + fixes in options tests Jul 31, 2025
@boegel boegel added the tests label Jul 31, 2025
@boegel boegel changed the title Fix error when adding files to existing PR and refactor + fixes in options tests Fix error when adding files to existing PR with --update-pr and refactor + fixes in options tests Jul 31, 2025
boegel
boegel requested changes Aug 1, 2025
View reviewed changes
Comment thread test/framework/options.py Outdated
boegel
boegel requested changes Aug 1, 2025
View reviewed changes
Comment thread test/framework/options.py Outdated
boegel
boegel previously approved these changes Aug 1, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm, will merge when CI gives green light after two very small changes to options test suite...

@boegel
Copy link
Copy Markdown
Member

boegel commented Aug 1, 2025 •
edited
Loading

One of the tests that's only run when a GitHub token is available was failing:

ERROR: test_github_from_pr_x (test.framework.options.CommandLineOptionsTest)
Test combination of --from-pr with --extended-dry-run.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/runner/6e37297d700749063c0656a77a44a236da67bd19/lib/python3.9/site-packages/test/framework/options.py", line 2216, in test_github_from_pr_x
    self._assert_regexs(msg_regexs, stdout)
  File "/tmp/runner/6e37297d700749063c0656a77a44a236da67bd19/lib/python3.9/site-packages/test/framework/options.py", line 4452, in _assert_regexs
    regex = re.compile(regex, re.M)
  File "/opt/hostedtoolcache/Python/3.9.23/x64/lib/python3.9/re.py", line 252, in compile
    return _compile(pattern, flags)
  File "/opt/hostedtoolcache/Python/3.9.23/x64/lib/python3.9/re.py", line 299, in _compile
    raise ValueError(
ValueError: cannot process flags argument with a compiled pattern

That's fixed in 563828f

boegel
boegel approved these changes Aug 1, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel boegel enabled auto-merge August 1, 2025 09:48
@boegel boegel merged commit f8c94c3 into easybuilders:develop Aug 1, 2025
37 checks passed
@Flamefire Flamefire deleted the 20250723180922_new_pr_wXUzyZFgvE branch August 4, 2025 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@boegel boegel boegel approved these changes

Assignees

No one assigned

Labels

bug fix tests

Projects

None yet

Milestone

5.1.2

Development

Successfully merging this pull request may close these issues.

--update-pr with nothing but a new easyconfig results in a catch 22

2 participants

@Flamefire @boegel