Skip to content

Conversation

@guewen
Copy link
Member

@guewen guewen commented Oct 24, 2022

Port of #403 from 14.0 to 15.0.

Can be tested on Runboat.

Open the runboat url with /queue_job/create_test_job?size=30.

image

@guewen guewen changed the base branch from 14.0 to 15.0 October 24, 2022 15:48
@guewen guewen force-pushed the oca-port-pr-403-from-14.0-to-15.0 branch 3 times, most recently from 54bc766 to a50f84b Compare October 26, 2022 07:41
Guewen Baconnier and others added 26 commits November 1, 2022 08:45
test_queue_job will create channels root.sub and root.sub.sub, so the
tests doing the same thing will fail with a unique constraint error.
When displayed in a tab, the widget show the nodes offset, the d3
network is probably confused by the size or visibility of its canvas.
Install a listener on tabs to trigger a fit() on the network.
A graph of jobs always share the same graph_uuid, which can be
used to group jobs, but is also a faster way to find all the jobs
of the graph. Then we can use the dependencies field to build the
whole graph from the pre-selection of jobs.
Jobs waiting that their dependencies are executed cannot be requeued.
They have to keep waiting their turn.
As jobs are created in the setup, but not always delayed,
the __del__ warning that the job was not delayed is triggered.
Create only the jobs necessary for each test.
* mock_jobs context manager with assert methods to check enqueued jobs
* jobs are not stored in database, yet they can be inspected and performed
  during the test
* the context manager works with graphs enqueued by delayable and with with_delay()
* direct execution of jobs works with graphs by executing them synchronously
  following their topological sort
The previous implementation was incomplete, as it was setting
the graph_uuid when `Job.add_depends()` was called. It had the
advantage of being correct when (for e.g. a test) manually created
a Job and called `add_depends()`, but it could not work when in
some situations involving a group where one of the job has no job
depending on it.

Also, remove ``Job.add_reverse_depends()`` which is not used.
* Fix a js bug when there is no graph.
* Use a single read group for multiple jobs
The fact that mocks are used is an implementation detail, and is actually
hidden to the end user of the test API.
on_done() is maybe a better choice, it is less ambiguous than then() (clearer
on the fact it happens on done()) and done() (less confusion with set_done()).
I'll do the renaming soon, unless you have other suggestions.

Ref: OCA#154 (comment)
@guewen guewen force-pushed the oca-port-pr-403-from-14.0-to-15.0 branch from a50f84b to 10253b4 Compare November 1, 2022 07:45
@guewen guewen marked this pull request as ready for review November 1, 2022 07:52

_logger.debug("%s enqueue depends started", job)
job.enqueue_waiting()
_logger.debug("%s enqueue depends done", job)
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this to enqueue dependencies cc @ivantodorovich

Copy link
Contributor

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

Thanks ❤️ !

Copy link

@leemannd leemannd left a comment

Choose a reason for hiding this comment

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

This is nice!

@OCA-git-bot
Copy link
Contributor

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). 🤖

@jeremi
Copy link

jeremi commented Nov 15, 2022

Any idea on when this will be merged? This is a great improvement!

@lmignon
Copy link
Contributor

lmignon commented Nov 15, 2022

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 15.0-ocabot-merge-pr-476-by-lmignon-bump-major, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Nov 15, 2022
Signed-off-by lmignon
@OCA-git-bot
Copy link
Contributor

@lmignon your merge command was aborted due to failed check(s), which you can inspect on this commit of 15.0-ocabot-merge-pr-476-by-lmignon-bump-major.

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.

@pedrobaeza
Copy link
Member

A new copier update is needed due to OCA/oca-addons-repo-template#170

@lmignon
Copy link
Contributor

lmignon commented Nov 15, 2022

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 15.0-ocabot-merge-pr-476-by-lmignon-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 173b6ca into OCA:15.0 Nov 15, 2022
@OCA-git-bot
Copy link
Contributor

@lmignon The merge process could not be finalized because an exception was raised: <class 'requests.exceptions.ReadTimeout'>: A connection-level exception occurred: HTTPSConnectionPool(host='api.github.com', port=443): Read timed out. (read timeout=4).

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.

7 participants