Test summary confusingly list tests marked as skip() under pending

[jeffwidman]

I recently learned that there's an actual difference between marking a test as pending() (test runs, expected to fail) vs skip() (test doesn't run).

However, this isn't clear from the test run summary:

# run test marked with skip()

Finished in 0.00332 seconds (files took 2.42 seconds to load)
1 example, 0 failures, 1 pending

I would have expected it to say instead:

1 example, 0 failures, 0 pending, 1 skipped

Clearly differentiating between "skipped" and "pending" in the test summary will reduce confusion (I literally had no idea they behaved different! Thought they were just aliases of each other...). It will also make it quicker to realize a slow test suite may just need a few pending failures marked as skipped until someone has time to address them.

output of rspec --version:

$ rspec --version
RSpec 3.12
  - rspec-core 3.12.1
  - rspec-expectations 3.12.2
  - rspec-mocks 3.12.3
  - rspec-support 3.12.0

[JonRowe]

Oof, I think this is a long overlooked hangover from 2.x to 3.x when we essentially renamed the pending example behaviour to skipped (where as it used to be a duality), the count here is actually skipped examples if memory serves, as pending examples are expected to fail (count as passed) or don't fail (count as failed)

[jeffwidman]

If the desired outcome of my example is:

1 example, 0 failures, 1 skipped

Ie, rename pending to skipped in the summary, then I'd be happy to submit a PR.

I'm fine pretty much whatever way makes it clear when tests are running/pending vs skipped.

Btw, I think pending is a neat feature, I wish more test ecosystems supported the concept.

[JonRowe]

Turns out my memory fails me and this count includes both skipped and "passing" pending examples, as they're both in the output for pending, so to seperate this we'd need to seperate that count and the output for them, which is larger than just a rename

[danielmbrasil]

Hey @JonRowe, I'd like to work on this. However, I'm new to this project, so I need some guidance.

I've noticed that "skipped" examples are marked both as "pending" and "skip" in lib/rspec/core/pending.rb. Is it intended for "skipped" examples to stay as "pending" too, or the approach here should be to completely separate them?

My (maybe naive) approach was to simply add a new count (i.e. #skipped_count) to the summary in lib/rspec/core/notifications.rb. So, the summary now outputs something as follows:

3 examples, 0 failures, 2 pending (1 skipped)

Which could be read as: "2 pending examples, out of which 1 was skipped".
This keeps the current behavior and adds more information about the "pending" and "skipped" examples.

What are your thoughts on this?

[jeffwidman]

@danielmbrasil can you clarify why you'd opt for

3 examples, 0 failures, 2 pending (1 skipped)

rather than

1 example, 0 failures, 0 pending, 1 skipped

??

If it's just implementation details, then we probably should ignore that and first decide what makes sense for the user... in this case, it'd be straightforward to implement either one since the latter could also be implemented by just adding a new #skipped_count and then calculating pending as the sum of the current "pending+skipped" count minus the skipped count.

Personally, I think that 2 pending (1 skipped) still conflates the concept of "pending" and "skipped" when in reality they are two distinctly different concepts... Skipped is not a subset of pending.

[danielmbrasil]

Hi @jeffwidman. So, from what I understood from the source code (correct me if I'm wrong), "skipped" tests are both marked as "pending" and "skip" in their metadata. Here's a code snipped from lib/rspec/core/pending.rb:

rspec-core/lib/rspec/core/pending.rb

Lines 124 to 127 in 5e04142

	def self.mark_skipped!(example, message_or_bool)
	Pending.mark_pending! example, message_or_bool
	example.metadata[:skip] = true
	end

Thus, the implementation as-is seems to consider "skipped" tests as a subset of "pending".

The summary could indeed be something like this

1 example, 0 failures, 0 pending, 1 skipped

by changing how this method

rspec-core/lib/rspec/core/notifications.rb

Lines 318 to 320 in 5e04142

	def pending_count
	@pending_count ||= pending_examples.size
	end

works and adding a #skipped_count. So, #pending_count would only consider the examples that have pending set to true and skip set to nil in their metadata, and #skipped_count would count the examples which have both pending and skip set to true in their metadata.

Both this approach and my suggestion don't seem to break anything as they're only introducing a new count to the summary.

I suggested showing the "skipped" count as a subset of "pending" because internally that seems to be the case. I agree with you that showing two different counts (since both "skip" and "pending" behave differently) looks better. However, I don't know which approach would work better in terms of compatibility. Again, I'm pretty new to RSpec so I might have understood something wrong 😄. I'd be glad to work on it, though!

[JonRowe]

Output wise I prefer 1 example, 0 failures, 0 pending, 1 skipped, if we can unmark examples as pending when skipped I think we should but I'm not entirely certain you will be able to, theres some internal mechanics you might have to change

[jeffwidman]

@danielmbrasil Curious if this is something you're still interested in working on?

[danielmbrasil]

Curious if this is something you're still interested in working on?

I guess unmarking examples as pending when skipped is way beyond my league at the moment, so I'm not working on this.