Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

has_pending_actions_due() performance improvements #1077

Open
wants to merge 10 commits into
base: trunk
Choose a base branch
from

Conversation

crstauf
Copy link
Contributor

@crstauf crstauf commented Aug 1, 2024

Closes #1036.

At time of creation, does not include 'claimed' => true: unclear if this is appropriate or not.

@barryhughes barryhughes requested review from a team and coreymckrill and removed request for a team August 13, 2024 23:02
# Conflicts:
#	classes/abstracts/ActionScheduler_Store.php
Copy link
Contributor

@coreymckrill coreymckrill left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @crstauf !

I'm trying to think of the best way to test if has_pending_actions_due() still works as expected with these changes. Any ideas?

At time of creation, does not include 'claimed' => true: unclear if this is appropriate or not.

We discussed this a bit more and decided it's probably better to not include this right now, so this is fine.

classes/abstracts/ActionScheduler_Store.php Outdated Show resolved Hide resolved
@crstauf
Copy link
Contributor Author

crstauf commented Aug 16, 2024

I'm trying to think of the best way to test if has_pending_actions_due() still works as expected with these changes. Any ideas?

@coreymckrill Kinda surprised that there are no unit tests for it, but looking at it, the changes would not impact the result, only the performance.

coreymckrill added a commit that referenced this pull request Aug 24, 2024
In #1077 we're making some performance improvements to the
`has_pending_actions_due` method, but there are no unit tests to
protect against regressions, so this is simply adding some. Because
the tests are in the abstract class, they will get run for each
data store type.
Copy link
Contributor

@coreymckrill coreymckrill left a comment

Choose a reason for hiding this comment

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

@crstauf Fair point, adding unit tests for this in #1092

classes/abstracts/ActionScheduler_Store.php Show resolved Hide resolved
barryhughes pushed a commit that referenced this pull request Sep 4, 2024
In #1077 we're making some performance improvements to the
`has_pending_actions_due` method, but there are no unit tests to
protect against regressions, so this is simply adding some. Because
the tests are in the abstract class, they will get run for each
data store type.
coreymckrill added a commit that referenced this pull request Sep 5, 2024
In #1077 we're making some performance improvements to the
`has_pending_actions_due` method, but there are no unit tests to
protect against regressions, so this is simply adding some. Because
the tests are in the abstract class, they will get run for each
data store type.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

has_pending_actions_due() performance improvements
2 participants