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

Add failing test for scheduling in bulk and enqueued_at #5162

Conversation

@adamniedzielski
Copy link
Contributor

@adamniedzielski adamniedzielski commented Jan 31, 2022

While working on #5158 I noticed an inconsistency with how enqueued_at is being handled with push_bulk.

I'm submitting this PR with a new failing test to demonstrate this inconsistency. The reason for it is here. push_bulk always adds enqueued_at. push only adds it conditionally.

I'm not 100% sure what's expected here, but it prevents me from unifying push and push_bulk 😅

@mperham
Copy link
Owner

@mperham mperham commented Jan 31, 2022

600 is not a valid value for at. at must be epoch seconds, so the value should always be over one billion.

You are correct, enqueued_at should be mutually exclusive with at since scheduling does not enqueue (unless the schedule time is less than now!).

@adamniedzielski
Copy link
Contributor Author

@adamniedzielski adamniedzielski commented Feb 1, 2022

@mperham I'm going down a rabbit hole and getting more confused in this hole 🐰

600 is not a valid value for at. at must be epoch seconds, so the value should always be over one billion.

Understood. But I copy-pasted it from the test above 😅

scheduling does not enqueue (unless the schedule time is less than now!).

If you look at the existing test above it proves exactly the opposite for push_bulk:

  1. 'at' => 600
  2. we push two jobs
  3. and the scheduled set grows by 2
  4. I checked and Time.now is not mocked in this test

Is this a bug with a test passing by mistake or am I missing something obvious in my rabbit hole?

@mperham mperham merged commit 7e54709 into mperham:main Feb 1, 2022
0 of 4 checks passed
@mperham
Copy link
Owner

@mperham mperham commented Feb 1, 2022

Ok, find and fix what you can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants