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

Revert broken fix and add failing specs #107

Closed
wants to merge 2 commits into from

Conversation

pik
Copy link
Contributor

@pik pik commented Aug 21, 2015

Revert for #105 which did not fix anything.
Add failing specs to master then it would be preferable for everyone to know that the current Master is broken.

(If this is not promptly being resolved #101 has been outstanding for a while now) (Currently our application is on 3.0.13, which still has unresolved problems but not as bad as the current Head).

@@ -71,10 +95,10 @@ def perform(_)

it 'enqueues previously scheduled job' do
QueueWorker.sidekiq_options unique: true
jid = QueueWorker.perform_in(60 * 60, 1, 2)
QueueWorker.perform_in(60 * 60, [1, 2])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test is still broken and isn't testing for a job moving from the scheduled to its normal queue.

Right now this line is creating a scheduled job with a single argument of a two element array and the later Sidekiq::Client.push is creating a job with two Fixnum arguments. It's not the same job and the test is only showing that two separate jobs can be created, not that the same job can move from the scheduled to a normal queue.

Changing this line to QueueWorker.perform_in(60 * 60, 1, 2) should be enough to demonstrate the issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that should be 1,2 not [1,2].

@pik pik force-pushed the revert-broken-fix branch from c95d5fb to 2dbf8a1 Compare August 22, 2015 12:12
@mhenrixon mhenrixon closed this in 745ebea Aug 29, 2015
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.

2 participants