OCA / queue

Asynchronous Job Queue
GNU Affero General Public License v3.0
183 stars 460 forks source link

[14.0] Fix trap_jobs() recordset comparison #537

Closed guewen closed 1 year ago

guewen commented 1 year ago

The trap_jobs() test helper accepts any model/recordset as long as the job function is the same, which is not enough to verify that the job has been delayed on the expected recordset or model.

Tested job functions are "bound methods" so we can (and should) compare their __self__.

Example of assertion that would be valid:

partner = self.env["res.partner"].create({"name": "foo"})
with trap_jobs() as trap:
  partner.with_delay().do_something(42)
  trap.assert_enqueued_job(
      self.env["res.partner"].do_something,
      args=(42,)
  )

Now, it would fail with an error message like:

E           AssertionError: Job <res.partner()>.do_something(42) with properties (channel=root.test, description=Do something, eta=15, identity_key=<function identity_exact at 0x7f01bfb54cb0>, max_retries=1, priority=15) was not enqueued.
E           Actual enqueued jobs:
E            * <res.partner(64,)>.do_something(42) with properties (priority=15, max_retries=1, eta=15, description=Do something, channel=root.test, identity_key=<function identity_exact at 0x7f01bfb54cb0>)
OCA-git-bot commented 1 year ago

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

guewen commented 1 year ago

/ocabot merge minor

I choose minor over patch because succeeding tests can now fail (these tests were not "correct" in the first place), there is no breaking change otherwise.

OCA-git-bot commented 1 year ago

On my way to merge this fine PR! Prepared branch 14.0-ocabot-merge-pr-537-by-guewen-bump-minor, awaiting test results.

OCA-git-bot commented 1 year ago

@guewen your merge command was aborted due to failed check(s), which you can inspect on this commit of 14.0-ocabot-merge-pr-537-by-guewen-bump-minor.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

guewen commented 1 year ago

/ocabot merge minor

OCA-git-bot commented 1 year ago

This PR looks fantastic, let's merge it! Prepared branch 14.0-ocabot-merge-pr-537-by-guewen-bump-minor, awaiting test results.

OCA-git-bot commented 1 year ago

@guewen your merge command was aborted due to failed check(s), which you can inspect on this commit of 14.0-ocabot-merge-pr-537-by-guewen-bump-minor.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

guewen commented 1 year ago

Oh, it fails because of https://github.com/odoo/odoo/commit/f0cab1bf847a851b355ff90292e332ba185d2a10

guewen commented 1 year ago

/ocabot merge minor

OCA-git-bot commented 1 year ago

What a great day to merge this nice PR. Let's do it! Prepared branch 14.0-ocabot-merge-pr-537-by-guewen-bump-minor, awaiting test results.

OCA-git-bot commented 1 year ago

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