Spec: add support for focus

[asterite]

This PR includes a series of refactors to Spec in order to be able to support focus. Then it adds supports for it.

From one of the commits description:

Before this commit an it block ran immediately. This makes it hard to implement other filtering mechanisms. In particular to implement something like RSpec's focus we need to first know whether there's any example marked with focus: true. Without collecting all examples first we can't do this.

focus: true

Now you can mark describe, context and it with focus: true.

When you do that, only the things that are marked with focus: true will run. This is really useful to, well, focus on one or several specs. One can usually pass a line number, but in my experience it sometimes happens that you add stuff at the top of the spec file and the the line numbers shift. Adjusting the numbers is a bit tedious.

When any spec is marked with focus: true a new message appears at the end of the spec run:

Only running focus: true

(in cyan)

This is because you might forget that you left a focus: true and don't understand why only just a couple of specs run.

Better line matching

Now an entire describe/context will run if you specify a line that the describe contains but no inner describe/it matches. This is similar to RSpec. And this was impossible to do without collecting things.

An example:

require "spec"

describe "foo" do
  describe "bar" do
    it "one" do
    end
# <-------- if you mark this line, the entire "bar" will run
    it "two" do
    end
  end

  describe "baz"
  end
end

Previously nothing ran.

tags

I didn't implement it yet. But with this in place #8068 should become much, much simpler to do.

Breaking change?

This is a breaking change because if you do something like:

foo = 1
it "..."  do
  foo.should eq(1)
end

foo = 2
it "..."  do
  foo.should eq(2)
end

then it will fail because each it runs at the end of the program and foo's value will have changed.

However, I think this isn't that bad:

• we only had two occurrences of this in the standard library so it's probably not common
• it's easy to fix
• RSpec works exactly like that too

Another thing: pending block will now be compiled but not ran. Apparently this wasn't the case before. In one spec we passed raw_url to URI but I have no idea what that is, so I removed it. See: #8178

[asterite]

Another thing this refactor will enable is running specs in random order: can't do that if they always run one after the other.

[j8r]

I don't understand the purpose of the focus argument: what will it adds when tagging specs will be possible?

[asterite]

You can just focus on a few specs. Tagging is more complex: you need to think about a name, pass a flag, etc. I personally don't find tags useful, but focus is very useful for quick development cycles.

[asterite]

So the workflow is this: I need to add something new somewhere. I write a new describe or it section and put focus: true on that. Then I can focus on that new feature. When everything's ready I can remove focus: true and see that everything else works well.

Or maybe I add a new feature and just a single test starts failing. I can focus on that, fix it, then remove it, etc.

[Blacksmoke16]

Some JS testing libs have this as well. Is quite helpful.

E.x. https://davidtang.io/2016/01/03/controlling-which-tests-run-in-jasmine.html#only-running-specific-tests--specs

[Fryguy]

Love this change specifically because in #8068 I'm finding the filtering rules very difficult to implement since the "it" is matched first before checking the contexts, and that made it hard when trying to match the RSpec filtering logic.

I do have 2 concerns however.

• Is memory going to be an issue? I'd rather have it the way you have it in this PR (particularly for the ability to do random order) but I'm concerned about the memory overhead of holding a collection of all examples which we didn't have before.
• I've finally figured out the RSpec filtering rules (what a pain!) and this doesn't seem to align with them. Additionally, the way it's written here I don't think we can align with them. That is, in this PR, the assumption is that all filters apply sequentially (e.g. filter by pattern, then location, etc), but the RSpec matching rules are much more complicated. See Add ability to tag specs #8068 (comment) for a breakdown. I would love to know your opinion on whether or not we should try to align with RSpec for these matching rules. Frankly, I think your way is much more intuitive.

[Fryguy]

Additionally, in RSpec, focus is basically just a tag which follows the tag rules (with special logic elsewhere to auto-add focus to the list of requested tags) Would it make more sense to treat is as such after #8068 is merged, and thus not introduce a specific class_property for it?

[asterite]

Is memory going to be an issue?

I don't think so. The overhead is just creating lists of objects. The std specs have like 9000 specs. Creating an array of 9000 elements is almost instantaneous compared to the time it takes to actually run the spec contents.

I've finally figured out the RSpec filtering rules (what a pain!) and this doesn't seem to align with them

I don't think we should copy RSpec here. The way it's done in this PR, and how it was done previously, is that each filter (location, focus, pattern, etc.) keeps some tests and removes others. Applying them in any order should give the same result: they are additive.

So if we introduce tags we just need to filter examples with or without the given tags.

[Fryguy]

Sounds good to me, and is what I was thinking as well...much more intuitive.

[RX14]

I love it! This is required for parallelizing specs too, once preview_mt lands.

The refactor is much cleaner.

[asterite]

@RX14 Thank you!

[yxhuvud]

@RX14 I think it is also a necessary step to implement bisect, for finding order dependent errors.

[asterite]

I was also thinking about adding before/after/around hooks, which now could be placed before it instances. You would still not be able to pass data from the hooks into the specs, but it might be useful for example to clear a DB or similar.

[straight-shoota]

Another thing: pending block will now be compiled but not ran. Apparently this wasn't the case before. In one spec we passed raw_url to URI but I have no idea what that is, so I removed it.

Is this really the behaviour we want? This is a breaking change for quite a few specs. I explicitly used this as a feature. It allows to define pending specs which uses code that is not currently working but supposed to be implemented/fixed at some point.

[straight-shoota]

An example from sass.cr:

  # TODO: find_file returns empty string
  pending "#find_file" do
    it "finds file" do
      Sass.new(include_path: INCLUDES_PATH).find_file("_simple.scss").should eq File.join(INCLUDES_PATH, "_simple.scss")
    end
  end

Sass#find_file is currently not implemented because it doesn't work. But I'd like to keep the spec.

I could obviously comment it out. But I don't see much sense in compiling the content of pending blocks. When it's not working anyway, it doesn't need to be compiled. And when I need to check something compiles, I put it in typeof().

[asterite]

I guess we can change it. I thought it was an inevitable side effect of the refactor, but I just need to make pending to yield but without yielding. I'll fix it later today.

[asterite]

Actually I won't do it. If someone wants to do it please go ahead.

[bcardiff]

There were some specs in the ecosystem that used at_exit to shutdown some resources. The issue is that the whole specs are run in an at_exit and the specs were run after the shutdown of the specs.

Luckily the workaround is for the let cleanup happen when the process finished in this case, or monkey patch the Spec.run. Having either ordering options in the at_exit or hooks in Spec would be ideal.

[asterite]

There were some specs in the ecosystem that used at_exit to shutdown some resources

I think that's wrong. We should probably add a before_all and after_all hooks to spec. Then users could run things right after specs finish running.

Also, we could probably add before(:each) and before(:all) inside each describe/context, now that we don't depend on the order (you can put them before or after it blocks, it's the same). There's no way to share variables between those hooks, like now, but it can still be useful for setting up and tearing down resources.

[Fryguy]

In addition to before(:all) and after(:all), rspec has before(:suite) and after(:suite) which might be more appropriate in some cases (such as those at_exit cases). I think crystal should eventually offer all 3 as well.

• :suite is for doing one-time setup and teardown for an entire run
• :all is around a particular context
• :each is around an individual example