Crystal: Feature: Add a hook for before/after all specs run.

[jwoertink]

After this PR landed, the way specs ran changed quite a bit. Now if you have any at_exit handler in your spec_helper file, that block is ran before your specs instead of at the end.

It's pretty common to have some code you run first thing before all of your specs, and then another bit when all specs have completed running. In the case of LuckyFlow we want to boot up a chromedriver instance, run our specs against that, then when all specs are done, we make sure to shut that process down as well as the HTTP server that's running.

It would be great to have something like:

# Runs once before any actual spec runs
Spec.before_suite do
  setup_test_stuff
end

# Runs once when all specs have completed running (filtered or not)
Spec.after_suite do
  shutdown_test_stuff
end

ref: https://gitter.im/crystal-lang/crystal?at=5d8ba89d63b06968fbea378d
ref: https://github.com/luckyframework/lucky/pull/918/files#diff-e6b95b08ee4663a22e90491d70a0cbdaR131-R154

[Blacksmoke16]

I also ran into something similar:

require "http/server"
require "http/client"

require "spec"

SERVER = HTTP::Server.new do |context|
  context.response.print "Hello world!\n"
end

def run_server(& : HTTP::Client -> Nil) : Nil
  client = HTTP::Client.new "localhost", 3000
  spawn { SERVER.listen(3000) }
  sleep 0.5
  yield client
ensure
  pp "ensure"
  SERVER.close
end

run_server do |client|
  describe "something" do
    it "works" do
      pp "run client"
      pp client.get "/"
    end
  end
end

# => ensure
# => run client

I would now have to use use the run_server method in every it block, while doable it's def a bit less clean and DRY.

[jwoertink]

Hmm.. Would that work in crystal 0.31 though? I was thinking since the spec blocks just get gathered up, it looks like it would run the server, collect the specs, close the server, then run the specs at close. Just a guess though.

[Blacksmoke16]

@jwoertink Right, that's why that example doesn't work on 0.31.0 anymore. By the time the client.get goes to run, the server is closed.

You would have to wrap each of your it blocks in the run_server method like

describe "something" do
  it "works" do
    run_server do |client|
      pp "run client"
      pp client.get "/"
    end
  end
end

[jwoertink]

Ah right. That makes sense.

[Blacksmoke16]

This is kinda where having like beforeEach/beforeAll and afterEach/afterAll would be helpful, like https://jasmine.github.io/api/3.5/global.

In addition to a global before/after.

[bcardiff]

In crystal-db a new kind of it is defined that yield a db instance. That means the resource is created for each it. But is less verbose.

(Sent from mobile)

[j8r]

Or we can wrap all the it inside a block:

def some_stuff(&block)
  init
  begin
    yield
  ensure
    clean
  end
end
some_stuff do |stuff|
  it ...
    stuff.should eq ...
  end
end

[asterite]

@j8r No, that doesn't work. init and clean will run before running it. it runs separately at the end of the program.

[elorest]

Has anyone found a solution for this yet? Executing all setup and teardown for every spec increases run time for large projects enormously. Like slower than 2010 Rails with RSpec :(

[asterite]

You mean, the crystal-db way? I think for crystal db a single connection should be made and each spec would check out a connection from the pool. If that's not the case it should be improved.

Or what do you mean?

[elorest]

@asterite My understanding of the "crystal-db way" is that it still creates required resources before each it.

@bcardiff wrote above:

In crystal-db a new kind of it is defined that yield a db instance. That means the resource is created for each it. But is less verbose.

This is a problem as there are a lot of specs in Amber for testing generators, which generate a project and then check that it was generated correctly per the option provided. It also checks that it compiles and runs. If we have to generate the project before every it it takes substantially longer to run.

All of these specs have worked fine for the last 2 years, but we're still debating how to get all of our tests to pass with crystal 0.31.x. Preferably we can find a way to do it similarly to before.

[bcardiff]

To clarify, I meant the specs that are used in each db driver. mysql and sqlite specs use the helper at https://github.com/crystal-lang/crystal-db/blob/master/src/spec.cr#L468 to write specs as https://github.com/crystal-lang/crystal-mysql/blob/master/spec/db_spec.cr . Is not something that affect specs using crystal-db.

[elorest]

Thanks. I'm looking through that. This isn't quite the same situation but could probably follow the same pattern.

In this example how would I have the file only be created and deleted once?

require "spec"

module FileHelper
  extend self
  def create_file
    puts "writing file"
    File.write("testfile.txt", "this is here")
  end

  def delete_file
    puts "deleting file"
    File.delete("testfile.txt")
  end
end

describe "outer_tests" do
  it "should be empty" do
    File.exists?("testfile.txt").should be_false
  end
end

describe "makefile" do
  Spec.before_each do
    FileHelper.create_file
  end

  it "works" do
    File.read("testfile.txt").should eq "this is here"
  end

  it "adds to file" do
    File.open("testfile.txt", "a") do |f|
      f << " too."
    end

    File.read("testfile.txt").should eq "this is here too."
  end

  Spec.after_each do
    FileHelper.delete_file
  end
end

[asterite]

I don't think it's good to share state between tests. For example this will prevent running them in parallel in the future (if we want to eventually do that).

I plan to add before_each, before_all, after_each, after_all and maybe around_each and around_all to spec, would that help?

[asterite]

(of course if we add those methods there would still not be a way to share things created between a hook and a spec, well, only through a name like a known filename in your example)

[asterite]

And again, I really wouldn't mind reverting the refactor that caused this breakage. I think that the spec before the refactor was simpler:

• things run from top to bottom
• if you want to do some set up for a group of specs, you just do it before the it blocks and clean up after the it blocks

The refactor might enable:

• parallel test runs
• randomized test runs
• focus
• maybe other things?

So maybe the simplicity was overall better. If we go this route (the current way) we'll need to add before_each, before_all, etc., which is what the original spec didn't want to have because of how convoluted things become this way.

What do you think?

Again, I really wouldn't mind reverting the refactor. After all we are not 1.0 yet and we are free to experiment, do and undo things.

[elorest]

@asterite I agree that it's generally not a good idea to share state between tests. However there are cases where setup and tear down are simply too expensive to do every time.

For example this will prevent running them in parallel in the future (if we want to eventually do that).

This could probably done with an option and parallel or random order tests would ignore before alls.

I plan to add before_each, before_all, after_each, after_all and maybe around_each and around_all to spec, would that help?

Probably. I'm used to before and after in minispec and rspec. In my opinion it would make sense for before_all, before_each, after_all, after_each to be run within the scope of the describe they're nested in rather than global with Spec.before_each. In the past we moved to just listing things from top to bottom in the spec and it was a good work around.

Personally I was happy enough with the way spec was before. I could see an advantage to going to something more standard though with befores confined to their describe blocks.

[asterite]

Yes, I mean hooks confined to each describe (and nested describes). It's really easy to implement now. I'll probably send a PR for that next week if I have time.

[elorest]

@asterite Thanks a lot. I really love working in the Crystal community and appreciate the part you play in keep crystal great while improving it. We've written a lot of crystal at my work including the Infotainment System and Instrument Cluster in these trucks and am excited about the future. https://www.youtube.com/watch?v=u4mnYkH8ntc

Maybe we'll just hold off on fixing the Amber CLI specs until the next release of crystal.

[asterite]

@elorest do you share variables between specs? What I mean is, create something before a group of it blocks and then use it inside them. I ask because in the current spec, even with hooks, that's not possible.

[elorest]

We don't share variables between specs. Within a describe block we're fine with them running in any order. However we have been defining multiple variables which are used by all it blocks so that the values don't have to be defined multiple times. For example:

describe "files" do
  filename = "testfile"

  it "reads file" do 
    #something with filename
  end

  it "adds to file" do
    #adds to file and checks.
  end
end

Is that what you're saying isn't possible? If that's the case it would still be easy enough to call a function to define them all in each block.

[asterite]

I think that's still possible. It's just that it's hard to do some clean up after tests finish, I think.

[asterite]

I was thinking how to implement let from RSpec, or at least something similar: how to have a variable be created once for each spec, or once for all specs.

With before_each we could do:

describe "foo" do
  a = 0
  before_each do
    a = rand(1..10)
  end

  it "..." do
    puts a # => 4
  end

  it "..." do
    puts a # => 9
  end
end

That's not bad but it requires a bit of boilerplate:

• we need to declare the local variable
• we need to initialize it to some initial, uninteresting, value
• we need to assign it a value in the before each

I was thinking of a macro let, like this:

macro let(var)
  {{var.id}} = uninitialized typeof({{yield}})

  before_each do
    {{var.id}} = {{yield}}
  end
end

Essentially, writing:

let(a) { rand(1..10) }

would expand to:

a = uninitialized typeof(rand(1..10))

before_each do
  a = rand(1..10)
end

But that doesn't work: if we write

describe "foo" do
  let(a) { rand(1..10) }

  it "..." do
    puts a
  end
end

then Crystal will think that a is a method call, not a variable, because it doesn't see a = ... anywhere in the code.

However, I just thought that we could have something like:

let a = rand(1..10)

so the parser will see a = ... and see it as a variable. It works!

So that's kind of nice because let is a super simple macro, so even if we use it like all over the place it will work fine.

But there are other problems:

• if you use a outside of an it you will read the uninitialized value, which might lead to a segfault
• there's no way to redefine a let in a nested scope (but that might be a feature)

I'm still scratching my head thinking how we can have a variable be shared and initialized every time before each or all specs in a describe section... it needs a bit more thought, but maybe it's possible!

Another idea is to abandon the idea of referring to the variable by just doing a and instead doing something like a.get: that would raise an exception if called outside of an it, and we could also lazily initialize the value. Maybe it's not that bad, and it would also make it clear that a isn't a regular variable but a spec-related variable (in big RSpec it's hard to find out which things that looks like xyz are local variables or let-variables).

[asterite]

So this is working:

require "spec"

class Spec::Var(T)
  def initialize
    @has_value = false
    @value = uninitialized T
  end

  def value=(@value : T)
    @has_value = true
  end

  def get : T
    raise "let-var used outside of `it`" unless @has_value
    @value
  end
end

macro let(assignment)
  {{assignment.target}} = Spec::Var(typeof({{assignment.value}})).new

  before_each do
    {{assignment.target}}.value = {{assignment.value}}
  end
end

describe "foo" do
  let a = rand(1..10)
  let b = a.get + 100

  it "lele" do
    p! a.get
    p! b.get
  end

  describe "coco" do
    let a = rand(30..40)

    it "lala" do
      p! a.get
      p! b.get
    end

    it "lele" do
      p! a.get
      p! b.get
    end
  end
end

I get:

a.get # => 4
b.get # => 104
.a.get # => 34
b.get # => 108
.a.get # => 32
b.get # => 110

(and I have local changes to support before_each)

I'm not sure using get is nice, plus if you don't use it it will refer to the Spec::Var thing, but maybe it's not thaaat bad...

[elorest]

If let is called at the top of describe block instead of inside of a hook, is there really any advantage to using let instead of:

describe "foo" do
  a = rand(1..100)
  b = a + 100

  it "lele" do
    p! a
    p! b
  end
end

Either way it looks very usable.

[asterite]

@elorest The difference is that in my example a will get a different random number in each it block, while in yours it's always the same value.

I'm also thinking that if let defines something that is lazily computed and for which you must call get to get its value, eventually it could be a per-spec instance so that its value is not shared with other specs to allow parallel runs... but I'm not super sure about that.

[j8r]

I actually do this to initiate a state for certain specs, then removing it at the end:

require "spec"
require "file_utils"

def spec_with_tempdir(directory : String = File.tempname)
  Dir.mkdir directory
  begin
    yield directory
  ensure
    FileUtils.rm_rf directory
  end
end

def spec_with_file
  spec_with_tempdir do |dir|
    file = (Path[dir] / "test").to_s
    File.write file, "data"
    yield file
  end
end

spec_with_file do |file|
  it "does something" do
    file.should be_a String
  end

  it "does another thing" do
    File.read(file).should eq "data"
  end
end

This example use a file, but in my side I also use this this to test different aspects of an object in read-only, or to use a temporary directory to do the the tests then removing it.

[asterite]

@j8r You do know that your example doesn't work well with the latest Crystal/spec version, right?

(I ask because you keep suggesting that solution, but that doesn't work anymore)

[elorest]

@asterite Yeah that would definitely be useful. Although it would be really easy to add that pattern with scoped before_each, before_all etc. Thanks for you help.

[j8r]

It is working @asterite, I have just re-checked.

[j8r]

Hum, yeah it's working when running the all suite but not when running specific specs like crystal spec test.cr:22.
It can be considered as an imperfect work-around.

[asterite]

@j8r if I run your example I get:

.E

Failures:

  1) does another thing

       Error opening file '/var/folders/10/wk6sdnpd4lg0wc7_b56q6ksr0000gn/T/20191006-99266-5gt0qa/test' with mode 'r': No such file or directory (Errno)

What Crystal version are you using?

Here's what happens: it will capture a block and at the end of the program it will be executed. So your spec_with_tempdir will create the directory, capture the it block, delete the directory, and later when it is executed the tempdir won't exist anymore.

[j8r]

Oh ok nevermind, you're right. This logic is only working for my use case, where I need a temporary directory to create an object, then the path can be trashed out.