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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spec: add `before_suite` and `after_suite` #8238

Merged
merged 2 commits into from Sep 26, 2019

Conversation

@asterite
Copy link
Member

asterite commented Sep 26, 2019

Fixes #8235

Also changes the way Spec.after_each run: they now run in reverse order.

I tried it in RSpec and it works the same, and it makes sense because one usually does this:

# In one part of the code
Spec.before_suite do
  # set up thing 1
end

Spec.after_suite do
  # tear down thing 1
end

# In some other part of the code
Spec.before_suite do
  # set up thing 2
end

Spec.after_suite do
  # tear down thing 2
end

and in the end, because of the order things are called, you get:

# set up thing 1
# set up thing 2
# tear down thing 2
# tear down thing 1

Same goes for before_each and after_each: they are normally registered in pairs one after the other.

No specs for this, sorry, but the code is relatively trivial.

馃挱 Now that I think about it, what happens if one hook fails with an exception? I just tried it in RSpec and you get something like "An error occurred in an after(:suite) hook" and then a nicely printed exception. We should probably do the same, but I'd like to do that in a separate PR (and it doesn't need to be there in 0.31.1, after all if you get a failure it's something wrong in your setup).

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor

Blacksmoke16 commented Sep 26, 2019

It would be nice if you could scope the before/after_each to only it blocks that are within the describe block it was defined. Having something that executes before every it block makes it far less useful.

Something like

describe Int do
  describe Int32 do
    Spec.before_each do
      puts "Int32"
    end
  
    it "should work" do
      # Prints Int32 before running
      # Do something  
    end
  end
  
  describe Int64 do
    Spec.before_each do
      puts "Int64"
    end
  
    it "should work" do
      # Prints Int64 before running
      # Do something  
    end
  end
end

EDIT: 馃憤 Works for me :)

@asterite

This comment has been minimized.

Copy link
Member Author

asterite commented Sep 26, 2019

@Blacksmoke16 Yes, I want to do that and I see no problem with that. However I don't want to add too many things for 0.31.1. I think this PR is the necessary minimum to make the ecosystem work again.

@asterite

This comment has been minimized.

Copy link
Member Author

asterite commented Sep 26, 2019

馃挱 I would also really like to have around filters, though I have no idea how to implement that yet.

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor

Blacksmoke16 commented Sep 26, 2019

I would also really like to have around filters, though I have no idea how to implement that yet.

What would that look like?

@asterite

This comment has been minimized.

Copy link
Member Author

asterite commented Sep 26, 2019

What would that look like?

describe "..." do
  around_each do |example|
    server = ...
    example.run
    server.stop
  end
end

though of course I have no idea how would server be reachable to each it... maybe if `server is a variable outside of that scope it can be done, though that would break the possibility of running specs in parallel in the future because of shared state.

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor

Blacksmoke16 commented Sep 26, 2019

Oh i see, so essentially something similar to

  describe Int32 do
    Spec.before_all do
      server = Server.new
    end

    Spec.after_all do
      server.stop
    end
  
    it "should work" do
      server.some_method 
    end
  end

Where it defines variables that all it block could use?

We do something similar to that at work where you declare your vars at the outer describe block, then just assign the value in the before_all/before_each.

@asterite

This comment has been minimized.

Copy link
Member Author

asterite commented Sep 26, 2019

Where it defines variables that all it block could use?

You can do that if you define a variable outside of the describe block, but it's not something that around would enable just because it's there. So in essence it's the same "hack" but instead of using two hooks (one before, one each) you just need one. Probably not useful...

Copy link
Member

bcardiff left a comment

It鈥檚 good as is. We can discuss further APIs for 0.32

@bcardiff bcardiff added this to the 0.31.1 milestone Sep 26, 2019
@jwoertink

This comment has been minimized.

Copy link
Contributor

jwoertink commented Sep 26, 2019

@asterite you're amazing! This boosts the core Spec API 10x 馃帀 馃尞

@asterite asterite merged commit 7ea8fa6 into crystal-lang:master Sep 26, 2019
5 of 6 checks passed
5 of 6 checks passed
ci/circleci: test_preview_mt Your tests failed on CircleCI
Details
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/before-after-suite branch Sep 26, 2019
@paulcsmith

This comment has been minimized.

Copy link
Contributor

paulcsmith commented Sep 26, 2019

So fast :D Thank you

bcardiff added a commit that referenced this pull request Sep 27, 2019
Spec: add `before_suite` and `after_suite`
@ashishbista ashishbista referenced this pull request Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.