Default example status to unknown if corrupt #2360

matrinox · 2016-12-12T18:49:06Z

If the examples.txt is corrupt, it should at least not crash

myronmarston · 2016-12-12T19:26:12Z

Can you add a spec for this?

xaviershay · 2017-01-15T17:46:35Z

@matrinox are you still interested in this? If not I can have a crack at it.

matrinox · 2017-01-16T03:40:26Z

@xaviershay go ahead :). Thanks!
@myronmarston Sorry, didn't see this until now

matrinox · 2017-03-20T18:30:43Z

No one has done tests for this so I will take a stab at it

matrinox · 2017-03-20T19:02:24Z

@myronmarston Spec added. Please review

myronmarston

It's a good start, @matrinox. I left some feedback.

myronmarston · 2017-03-23T17:20:28Z

spec/rspec/core/configuration_spec.rb

+        end
+        expectation.not_to raise_error
+      end
+    end


A few things...

Since the failure is in last_run_statuses and we already have some specs specifically for that method, I'd prefer this spec was moved to there and re-written to exercise that directly. The specs I'm referring to are here.

Not raising an error is a good thing, but I think we can make the assertion even stronger; specifically that the status is set to UNKNOWN_STATUS. Otherwise, a fix of example.fetch(:status, "BWAH HA HA") could still pass, but the rest of the code in RSpec does not understand that status. So in your re-written spec, please assert that the status is set to UNKNOWN_STATUS.

I think in the other spec file you can get by w/o having to write an external fixture like you did here--instead you can just pass hashes to the simulate_persisted_examples method w/o a :status key. That helps keeps everything necessary to understand the spec visible in one place.

Thanks for the feedback. I am not super familiar with the rspec library so I just tried my best to put it where it's already kind of testing it.

is very good for sure. My main concern was just that it didn't crash but I suppose that it should be unknown, not just nil. That might make me change the code some more

Ok thanks! I'll try my best to get that working

matrinox · 2017-03-23T20:50:11Z

Can someone explain to me L107 of spec/rspec/core/configuration/only_failures_support_spec.rb? For context:

          expect(spec_files_with_failures).to(
            be_an(Array) &
            be(spec_files_with_failures) &
            contain_exactly("./spec_1.rb", "./spec_5.rb")
          )

Expect x to be x? Is that a typo?

----------------------------------------------------------- On branch master - Mon 12 Dec 2016 10:49:57 PST by matrinox <geoff.lee@lendesk.com>

----------------------------------------------------------- On branch master - Mon 20 Mar 2017 12:01:23 PDT by matrinox <geofflee25@gmail.com>

matrinox · 2017-03-23T21:03:59Z

I know this isn't related but what's the purpose of 'UNKNOWN_STATUS' beyond defaulting to the old status? If the file itself has an unknown status, then it may not run it despite it potentially being a failure. Especially if it's been corrupted/removed.

My proposal is to set unknown statuses as failure (or treat them as such) when loading from file. Thoughts?

myronmarston · 2017-03-23T21:46:00Z

I know this isn't related but what's the purpose of 'UNKNOWN_STATUS' beyond defaulting to the old status?

Are you asking what's the purpose of using UNKNOWN_STATUS as a constant instead of just using a literal 'unknown' string every where? Or why we are using that status at all?

We use a constant because it guards against typos, and saves a bit of memory by re-using the same string instance.

We use this status for examples for which we do not know the status because....what else would se use? If the status is unknown, it makes sense to label it as such. It also allows for filtering from the command line:

rspec --tag last_run_status:unknown

See #2352 for an example of where this is useful.

My proposal is to set unknown statuses as failure (or treat them as such) when loading from file. Thoughts?

I think that's not a good idea because there's no reason to assume examples with an unknown status are failures. For example, maybe the example has never run on this machine before because the repo was just cloned and not all examples ran on the first run (e.g. due to a filter, or due to --fail-fast or whatever). Setting the last_run_status to unknown for all examples for which we do not know the status allows someone to filter to just them from the command line, as I mention above.

@myronmarston

Made changes based on @myronmarston feedback ----------------------------------------------------------- On branch master - Thu 23 Mar 2017 14:46:27 PDT by matrinox <geofflee25@gmail.com>

myronmarston · 2017-03-23T21:48:56Z

Expect x to be x? Is that a typo?

It's not a typo. We are using that to require memoization. expect([]).to be([]) fails because while their contents are the same, they are not the same array instance. So that part of the expectation requires that the method returns the same array instance when called a 2nd time, rather than taking the time to re-compute the array from scratch.

matrinox · 2017-03-24T17:13:31Z

It's almost the weekend so I might work on it then but for now, to speed things up a little, do you mind explaining why it's failing only on a jruby configuration? Or how I can easily set it up on my Mac. Thanks

myronmarston · 2017-03-24T17:15:44Z

It's almost the weekend so I might work on it then but for now, to speed things up a little, do you mind explaining why it's failing only on a jruby configuration? Or how I can easily set it up on my Mac. Thanks

The bisect cukes randomly fail on JRuby occasionally and we don't know why. Probably a JRuby bug given they pass on MRI 100% of the time. Regardless, I've kicked the JRuby build and we'll see if it goes green. That's how we always deal with it.

matrinox · 2017-03-24T20:38:03Z

@myronmarston looks good now. Welcome feedback

myronmarston · 2017-03-27T16:23:06Z

Thanks!

@myronmarston

* Default example status to unknown if corrupt ----------------------------------------------------------- On branch master - Mon 12 Dec 2016 10:49:57 PST by matrinox <geoff.lee@lendesk.com> * Add spec for when examples.txt file are corrupted ----------------------------------------------------------- On branch master - Mon 20 Mar 2017 12:01:23 PDT by matrinox <geofflee25@gmail.com> * Default status to UNKNOWN_STATUS if it's valid or nil Made changes based on @myronmarston feedback ----------------------------------------------------------- On branch master - Thu 23 Mar 2017 14:46:27 PDT by matrinox <geofflee25@gmail.com>

@myronmarston

* Default example status to unknown if corrupt ----------------------------------------------------------- On branch master - Mon 12 Dec 2016 10:49:57 PST by matrinox <geoff.lee@lendesk.com> * Add spec for when examples.txt file are corrupted ----------------------------------------------------------- On branch master - Mon 20 Mar 2017 12:01:23 PDT by matrinox <geofflee25@gmail.com> * Default status to UNKNOWN_STATUS if it's valid or nil Made changes based on @myronmarston feedback ----------------------------------------------------------- On branch master - Thu 23 Mar 2017 14:46:27 PDT by matrinox <geofflee25@gmail.com>

@myronmarston

…#2360) * Default example status to unknown if corrupt ----------------------------------------------------------- On branch master - Mon 12 Dec 2016 10:49:57 PST by matrinox <geoff.lee@lendesk.com> * Add spec for when examples.txt file are corrupted ----------------------------------------------------------- On branch master - Mon 20 Mar 2017 12:01:23 PDT by matrinox <geofflee25@gmail.com> * Default status to UNKNOWN_STATUS if it's valid or nil Made changes based on @myronmarston feedback ----------------------------------------------------------- On branch master - Thu 23 Mar 2017 14:46:27 PDT by matrinox <geofflee25@gmail.com> --- This commit was imported from rspec/rspec-core@6fdff12.

This commit was imported from rspec/rspec-core@82ad6ce.

matrinox force-pushed the master branch from 93b9b3f to 86a655f Compare December 12, 2016 18:50

xaviershay added the Bug label Jan 15, 2017

myronmarston requested changes Mar 23, 2017

View reviewed changes

geoff-lee-lendesk and others added 2 commits March 23, 2017 13:52

Default example status to unknown if corrupt

99ad590

----------------------------------------------------------- On branch master - Mon 12 Dec 2016 10:49:57 PST by matrinox <geoff.lee@lendesk.com>

Add spec for when examples.txt file are corrupted

8a38e97

----------------------------------------------------------- On branch master - Mon 20 Mar 2017 12:01:23 PDT by matrinox <geofflee25@gmail.com>

matrinox force-pushed the master branch from aed7497 to 8a38e97 Compare March 23, 2017 20:52

Default status to UNKNOWN_STATUS if it's valid or nil

8862a61

Made changes based on @myronmarston feedback ----------------------------------------------------------- On branch master - Thu 23 Mar 2017 14:46:27 PDT by matrinox <geofflee25@gmail.com>

myronmarston approved these changes Mar 27, 2017

View reviewed changes

myronmarston merged commit ddd8ff3 into rspec:master Mar 27, 2017

myronmarston added a commit that referenced this pull request Mar 27, 2017

Follow up to #2360.

e121f3d

JonRowe pushed a commit that referenced this pull request Apr 20, 2017

Follow up to #2360.

82ad6ce

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Follow up to rspec#2360.

32628e1

yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 2021

[core] Follow up to rspec/rspec-core#2360.

36216fb

This commit was imported from rspec/rspec-core@82ad6ce.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default example status to unknown if corrupt #2360

Default example status to unknown if corrupt #2360

matrinox commented Dec 12, 2016 •

edited

Loading

myronmarston commented Dec 12, 2016

xaviershay commented Jan 15, 2017

matrinox commented Jan 16, 2017 •

edited

Loading

matrinox commented Mar 20, 2017

matrinox commented Mar 20, 2017

myronmarston left a comment

myronmarston Mar 23, 2017

matrinox Mar 23, 2017 •

edited

Loading

matrinox commented Mar 23, 2017

matrinox commented Mar 23, 2017

myronmarston commented Mar 23, 2017

myronmarston commented Mar 23, 2017

matrinox commented Mar 24, 2017

myronmarston commented Mar 24, 2017

matrinox commented Mar 24, 2017

myronmarston commented Mar 27, 2017

Default example status to unknown if corrupt #2360

Default example status to unknown if corrupt #2360

Conversation

matrinox commented Dec 12, 2016 • edited Loading

myronmarston commented Dec 12, 2016

xaviershay commented Jan 15, 2017

matrinox commented Jan 16, 2017 • edited Loading

matrinox commented Mar 20, 2017

matrinox commented Mar 20, 2017

myronmarston left a comment

Choose a reason for hiding this comment

myronmarston Mar 23, 2017

Choose a reason for hiding this comment

matrinox Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

matrinox commented Mar 23, 2017

matrinox commented Mar 23, 2017

myronmarston commented Mar 23, 2017

myronmarston commented Mar 23, 2017

matrinox commented Mar 24, 2017

myronmarston commented Mar 24, 2017

matrinox commented Mar 24, 2017

myronmarston commented Mar 27, 2017

matrinox commented Dec 12, 2016 •

edited

Loading

matrinox commented Jan 16, 2017 •

edited

Loading

matrinox Mar 23, 2017 •

edited

Loading