-
Notifications
You must be signed in to change notification settings - Fork 132
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
Testing the negative case of have_enqueued_sidekiq_job with any arguments? #165
Comments
I've solved similar problems by asserting on how many jobs were queued, sometimes ensuring no jobs were queued. For example: expect(MyJob.jobs.size).to eq(0) |
I also hit the same problem and was very surprised, after writing all my green tests with In the end I used the same hack suggested by @cgunther, except I prefer this version because it gives you the details of enqueued jobs when it fails: expect(MyJob.jobs).to eq([]) But I believe this is quite misleading and unexpected behavior. If there are good reasons to not support this behavior, it should now be allowed and raise when writting such tests IMO. |
Thinking more on this, I don't think it's specific to the negative case, but rather confusion around what the argument-less For example, consider the following: class MyJob
include Sidekiq::Job
def perform(id)
# ...
end
end
MyJob.perform_async(1)
expect(MyJob).to have_enqueued_sidekiq_job Do we expect that to pass or fail? Currently it fails because it's looking for an argument-less job having been enqueued (ie. Applying the line of thinking from this thread, one might expect it to pass on the premise that at least one job was enqueued, regardless of arguments (since none were provided). For whatever it's worth, the README does note that it's for specified arguments, but we could expand on that to make it clearer: Or add a note about preferring a Ultimately I think the only way we could make the problematic examples raise is if we inspected the job/worker class's |
@cgunther Indeed that example you gave is less common for me (because when I test the presence of a Job I want to make sure it's the right one too so I check arguments) but I also find it unatural. This is may be habits but most methods I know in the expect { }.to raise_error # does pass if any exception is raised
expect { }.not_to raise_error # does fail if any exception is raised
# And that is the recommended way for less brittle tests And closer to this one the ActiveJob matcher also works in this same precictable way: expect { }.to enqueue_job # does pass if any job is enqueued
expect { }.not_to enqueue_job # does fail if any job is enqueued There are probably more examples in the rspec world, I believe ruby developers are using to this and the principle of least astonishment may prime if the "it's logical to me" argument is not decisive enough. |
It'd probably be a big migration for existing users of this library, but maybe rspec-rails's native I'm just a fellow user of this library, so it'd probably be up to someone to take a stab at this in a PR. |
Indeed, that's another good example :) I could take a stab at a PR but I am already spending a lot energy writting PRs and maintaing forks for abandonned projects that I'll only do it with the approval of the maintainer first (so if I know it has a chance to be merged someday). |
Not that it's exactly solving this use case but #200 would add support for the builtin argument matchers, so you could do something like: expect(SomeJob).not_to have_enqueue_sidekiq_job(any_args) It's a little more verbose (and doing any negation math when reading an expectation might still cause confusion), but I think that's probably the best solution for now. I can add a note in the README |
Piling on, with the new block syntax this works and is a little more intuitive. expect { "do nothing" }.not_to enqueue_sidekiq_job |
I like this block version indeed 👍 |
Just because there was a long time between versions, I was aiming to not disrupt existing projects and their use of the existing matchers. In future versions (since I'll be maintaining this going forward), I'll look into making the |
I'm trying to test that a job was not enqueued. When I write the test like so, the test passes even though the job was enqueued:
If I write it with the specific arguments that it is enqueued with, it will fail. I don't want to specify arguments because if they change, my test will not be valid.
Specifying anything for each arguments gives the result I want, but I would prefer not to have to explicitly state anything about the value or number of arguments for my test.
I would expect using any_args would work, but it does not work in either case:
What is the correct way to test that a job was not enqueued without having to provide any information about the arguments it might receive?
The text was updated successfully, but these errors were encountered: