Negative matcher should be failed if have_enqueued_job have 1 or more jobs

[awcodify]

Fixing issue #1870

Examples:

ExampleJobs.perform_later
ExampleJobs.perform_later

expect(ExampleJobs).not_to have_enqueued_jobs(ExampleJobs)

It should be failed, but in our current negative matcher above will be passed because we have more than one jobs and in our current code is checking for exactly one jobs.

We can hack current state without changing any code with:

expect{ my_job.perform }.to have_enqueued_job(OtherJob).exactly(0).times

but it's wierd.

[JonRowe]

This changes the behaviour of the positive version, whats needed is to change how this is checked when negated only.

[awcodify]

@JonRowe when we initialize this matcher without any arguments it will be at least one, not exactly one. So, it will change the behaviour of the positive version too.

[JonRowe]

@whatdacode no it's exactly once to match the semantics of other rspec matchers.

[awcodify]

@JonRowe Sorry, what do you mean about semantics? Incremental of jobs count?

[JonRowe]

@awcodify Semantics is the meaning of something. The matchers which perform similar tasks to this one use exactly once as a default, thus the positive version of this matcher needs to maintain its exactly once default in order to maintain its meaning. You cannot change its default behaviour.

[Istanful]

@JonRowe Would you care to elaborate why the default is "exactly once"? I totally agree to be consequent but I'm curious as to why that would be more correct.

From reading the following specs I think it would be correct to assume "at least once":

# Should pass, does pass.
expect {
  MyJob.perform_later
}.to have_enqueued_job(MyJob)

# Should pass, does not.
expect {
  MyJob.perform_later
  MyJob.perform_later
}.to have_enqueued_job(MyJob)

# Should fail, does not.
expect {
  MyJob.perform_later
  MyJob.perform_later
}.not_to have_enqueued_job(MyJob)

This goes for the have_received matcher as well:

# Should pass, does pass.
my_spy = spy('spy')

my_spy.expected_method

expect(my_spy).to have_received(:expected_method)

# Should pass, does not.
my_spy = spy('spy')

my_spy.expected_method
my_spy.expected_method

expect(my_spy).to have_received(:expected_method)

# Should fail, does not.
my_spy = spy('spy')

my_spy.expected_method
my_spy.expected_method

expect(my_spy).not_to have_received(:expected_method)

[JonRowe]

Would you care to elaborate why the default is "exactly once"?

As you have shown this is ambiguous as to how it is interpreted, but which is correct is a matter of opinion. The current code has an established behaviour and thus we cannot change it without a major release and considering it is a matter of interpretation it won't change.

Additionally consider that:

my_spy.expected_method
my_spy.expected_method

# or

MyJob.perform_later
MyJob.perform_later

Is more likely to be a mistake, such it leads weighting towards making the default "exactly" once, if you want to do something more than once you generally specify it.

Additionally I am ok with fixing this so:

expect {
  MyJob.perform_later
  MyJob.perform_later
}.not_to have_enqueued_job(MyJob)

Fails, it just can't change the positive behaviour

[Istanful]

@JonRowe Sounds fair enough. Personally I think that my version should be the default, since it more accurately does what it implies. I can however see why you would not change it in the current state of the gem.

@awcodify Can I implement the negated matcher or do you want to do it? :)

[JonRowe]

Personally I think that my version should be the default, since it more accurately does what it implies.

Thats is, as prefaced, your opinion. It is not everyones opinion. Personally I think its more accurate as it is.

[Istanful]

@JonRowe Definitely. No hard feelings at all. :)

[awcodify]

Like @Istanful said, "I totally agree to be consequent but I'm curious as to why that would be more correct."

@JonRowe Simply, it means we can't change our long established behaviour, right? So, this is a known as issue (at least for me), but don't 👍 . And we can't change because of major behaviour?

Still we need this PR or not?

[Istanful]

@awcodify Exactly. He said it would be fine to make an exception for this matcher though. Basically implementing a custom version of the negated matcher to make it work as intended.

Do you want to implement this in another PR or should I? :)

[krzysiek1507]

Any update on this? ;)

[Istanful]

@krzysiek1507 Still waiting for an answer from @awcodify. :)

[krzysiek1507]

@Istanful, I hope @awcodify won't mind if you do it. ;)

[Istanful]

@krzysiek1507 @awcodify Alright. I'm gonna get cracking on this then. :)

[Istanful]

@JonRowe I just opened a PR for this. #2069

[krzysiek1507]

Thanks!

[JonRowe]

Closed if favour of #2069