Skip to content

Fixing tests broken by introduction of check for race condition#2998

Merged
centosinfra-prod-github-app[bot] merged 2 commits intopackit:mainfrom
jpodivin:fix/tests/again
Feb 26, 2026
Merged

Fixing tests broken by introduction of check for race condition#2998
centosinfra-prod-github-app[bot] merged 2 commits intopackit:mainfrom
jpodivin:fix/tests/again

Conversation

@jpodivin
Copy link
Contributor

@jpodivin jpodivin commented Feb 16, 2026

Recent changes related to race condition issue have broken database tests.

The test case test_copr_build_race_condition_concurrent_packages introduced f395aac was created as intentionally failing, without being marked as such. That is confusing, since it would mess up the report, but it also replaced SRPMBuildModel.create_with_new_run with a non-standard mock.

Since this mock method was not inserted by flexmock, or other mocking utility, it had effect on all tests that followed. Leading to their failure.

There was also a minor issue, in that the too_many_copr_builds fixture was not modified, when the return value of CoprBuildGroupModel.create method was changed by 7e99633.

@jpodivin jpodivin requested a review from a team as a code owner February 16, 2026 08:54
@jpodivin jpodivin requested review from nforro and removed request for a team February 16, 2026 08:54
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request addresses several issues in the test suite that were introduced by recent changes. The fixes include correctly marking an intentionally failing test with @pytest.mark.xfail, replacing a manual monkeypatch with a properly scoped flexmock mock to prevent state leakage between tests, and updating a fixture to match a changed method signature. These changes are well-justified and improve the overall health and stability of the test suite. I've found one minor but important typo in the test code that needs to be fixed.

@centosinfra-prod-github-app
Copy link
Contributor

@centosinfra-prod-github-app
Copy link
Contributor

@centosinfra-prod-github-app
Copy link
Contributor

Copy link
Member

@nforro nforro left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@majamassarini majamassarini left a comment

Choose a reason for hiding this comment

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

Thanks, I didn't realize I broke other tests. I checked and, using flexmock, the test still triggers the race condition, so LGTM.
However, I will keep the comments since this is a complex unit test and I think they are useful.

@centosinfra-prod-github-app
Copy link
Contributor

@centosinfra-prod-github-app
Copy link
Contributor

@centosinfra-prod-github-app
Copy link
Contributor

Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
@majamassarini majamassarini added the mergeit Merge via Zuul label Feb 26, 2026
@centosinfra-prod-github-app
Copy link
Contributor

@centosinfra-prod-github-app
Copy link
Contributor

@centosinfra-prod-github-app centosinfra-prod-github-app bot merged commit 43fea6c into packit:main Feb 26, 2026
6 checks passed
@github-project-automation github-project-automation bot moved this from New to Done in Packit pull requests Feb 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mergeit Merge via Zuul

Projects

Development

Successfully merging this pull request may close these issues.

3 participants