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
Copy link
Copy Markdown
Member

@TDu TDu commented Jun 4, 2026

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!

@OCA-git-bot OCA-git-bot added series:18.0 mod:queue_job_profiler Module queue_job_profiler labels Jun 4, 2026
Copy link
Copy Markdown
Member

@amh-mw amh-mw 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 only, LGTM. Bonus points still available for a unit test that would have noticed the method signature change.

@simahawk
Copy link
Copy Markdown
Contributor

simahawk commented Jun 4, 2026

@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 TDu force-pushed the 18-queue-job-profiler-adapt-to-refactor-job-aquisition branch 2 times, most recently from c5e8590 to ffa8ca2 Compare June 5, 2026 09:30
@TDu
Copy link
Copy Markdown
Member Author

TDu commented Jun 5, 2026

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

if not config["test_enable"]:

Copy link
Copy Markdown
Contributor

@simahawk simahawk left a comment

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 TDu force-pushed the 18-queue-job-profiler-adapt-to-refactor-job-aquisition branch from ffa8ca2 to ccef05c Compare June 5, 2026 12:27
@simahawk
Copy link
Copy Markdown
Contributor

simahawk commented Jun 5, 2026

/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.0 Jun 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 free to 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