Skip to content

[18.0][FIX] Adapt queue_job_profiler to job aquisition refactor - #941

Merged
OCA-git-bot merged 1 commit into
OCA:18.0from
camptocamp:18-queue-job-profiler-adapt-to-refactor-job-aquisition
Jun 5, 2026
Merged

[18.0][FIX] Adapt queue_job_profiler to job aquisition refactor#941
OCA-git-bot merged 1 commit into
OCA:18.0from
camptocamp:18-queue-job-profiler-adapt-to-refactor-job-aquisition

Conversation

@TDu

@TDuTDu commented Jun 4, 2026

Copy link
Copy Markdown
Member

That module was develop before this change

The new module tests were not affected by the change when merging.

@OCA-git-bot

Copy link
Copy Markdown
Contributor

Hi @simahawk,
some modules you are maintaining are being modified, check this out!

@amh-mwamh-mw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code review only, LGTM. Bonus points still available for a unit test that would have noticed the method signature change.

@simahawk

Copy link
Copy Markdown
Contributor

@TDu can you try to add the test?
As you are there, can you pls rewrite the commit msg to something like

[FIX] queue_job_profiler: controller hook
Broken by job acquisition refactoring #941

?

TDu added a commit to camptocamp/queue that referenced this pull request Jun 5, 2026
Broken by job acquisition refactoring OCA#941
@TDu
TDuforce-pushed the 18-queue-job-profiler-adapt-to-refactor-job-aquisition branch 2 times, most recently from c5e8590 to ffa8ca2CompareJune 5, 2026 09:30
@TDu

TDu commented Jun 5, 2026

Copy link
Copy Markdown
MemberAuthor

About adding a test that would have cached the change and make the module fail. I agree it would have been good to have one, but I am not sure of the pertinence of adding it afterwards.
For such test to make sense it would need to call the _run_job function and render the test suite more complex.

But I have updated the tests to use the RunJobController class and not an instance of it.
And remove the commit mock, because it is not needed IMO

ifnotconfig["test_enable"]:

@simahawksimahawk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@TDu pls squash

Broken by job acquisition refactoring OCA#941
@TDu
TDuforce-pushed the 18-queue-job-profiler-adapt-to-refactor-job-aquisition branch from ffa8ca2 to ccef05cCompareJune 5, 2026 12:27
@simahawk

Copy link
Copy Markdown
Contributor

/ocabot merge patch

@OCA-git-bot

Copy link
Copy Markdown
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 18.0-ocabot-merge-pr-941-by-simahawk-bump-patch, awaiting test results.

@OCA-git-bot
OCA-git-bot merged commit 0525583 into OCA:18.0Jun 5, 2026
7 checks passed
@OCA-git-bot

Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at 71220ea. Thanks a lot for contributing to OCA. ❤️

Sign up for freeto 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.

4 participants

@TDu@OCA-git-bot@simahawk@amh-mw