Skip to content
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

Spec: add support for `focus` #8125

Merged
merged 8 commits into from Sep 9, 2019

Conversation

@asterite
Copy link
Member

commented Aug 29, 2019

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 added 6 commits Aug 28, 2019
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.

So, this commit first collects all describes, contexts and examples and
then filters then. The code is now much cleaner is easier to extend.
@asterite asterite force-pushed the asterite:spec/focus branch from 8c72adc to a5b1553 Aug 29, 2019
@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

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

@asterite

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

src/spec/context.cr Outdated Show resolved Hide resolved
src/spec/item.cr Show resolved Hide resolved
@Fryguy

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

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 #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

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

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

@RX14
RX14 approved these changes Sep 8, 2019
Copy link
Member

left a comment

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

The refactor is much cleaner.

@asterite asterite added this to the 0.31.0 milestone Sep 9, 2019
@asterite

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

@RX14 Thank you!

@asterite asterite merged commit cceafa9 into crystal-lang:master Sep 9, 2019
5 checks passed
5 checks passed
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asterite asterite deleted the asterite:spec/focus branch Sep 9, 2019
@yxhuvud

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

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

@asterite

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

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.

@j8r j8r referenced this pull request Sep 11, 2019
@straight-shoota

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

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

@bcardiff

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2019

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
@bcardiff bcardiff referenced this pull request Sep 23, 2019
jessedoyle added a commit to jessedoyle/duktape.cr that referenced this pull request Sep 25, 2019
- Fix test cases that were failing because Crystal's Spec library
  now executes `it` blocks at the end of the program
  (crystal-lang/crystal#8125). Instead of
  manually destroying the Duktape head in specs, let the GC take
  care of it.
- Update `ameba` to 0.10.1.
@jessedoyle jessedoyle referenced this pull request Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.