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

before_each should be scoped within describe block #5094

Closed
georgeu2000 opened this issue Oct 9, 2017 · 16 comments
Closed

before_each should be scoped within describe block #5094

georgeu2000 opened this issue Oct 9, 2017 · 16 comments

Comments

@georgeu2000
Copy link

I'm not sure if this is by design, but this behavior was very surprising to me, and different from RSpec.

describe "Part 1" do
  Spec.before_each do
    puts "before_each"
  end
  it "success" do
    ...
  end
end

describe "Part 2" do
  it "success" do
    ...
  end
end

I expect that the before_each block defined in Part 1 only runs before specs in Part 1. But it seems to run before each spec that is defined after the before_each block, even if they are outside of the Part 1 describe block.

According to the docs,

Instructs the spec runner to execute the given block before each spec, regardless of where this method is invoked.

So, it may be that it is designed to work outside of the describe block. But it does not work as the docs describe either: it only runs on specs defined after the before_each block

@akzhan
Copy link
Contributor

akzhan commented Oct 9, 2017

Yes, I'm too frustrated by strange :each blocks.

@georgeu2000
Copy link
Author

I took a look at the source, but didn't see how to update it.

@bew
Copy link
Contributor

bew commented Oct 10, 2017

The *_each methods adds blocks globally (see comment #4304 (comment)), not in a spec scope.

@akzhan
Copy link
Contributor

akzhan commented Oct 10, 2017

@bew yes, it's now. But it must be reimplemented in classic setup/teardown way (see #4304 (comment)).

@ysbaddaden
Copy link
Contributor

This is by design.

Duplicate of #4304.

@straight-shoota
Copy link
Member

straight-shoota commented Oct 10, 2017

@ysbaddaden Is there an explanation for why this is by design? I honestly can't think of a reason why you would want to have code run before/after every spec in a suite that may have hundreds.

And this is not a duplicate: This issue here is about scoping before/after hooks to certain example groups while #4304 is essentially about execution order that before/after hooks don't run for specs that are lexically defined before them. Please reopen.

@ysbaddaden
Copy link
Contributor

Please search the github issues, there has been explanations about this. I don't use spec myself but minitest, so I won't search further than my memory.

@straight-shoota
Copy link
Member

I see no sense in just stating "This is by design" and tagging as a wrong duplicate. This helps no-one to get any wiser.

For reference: There is #1378 which proposed a specific implementation but was discarded due to performance problems. And there is #167 which suggested a number of features for spec but was closed because @asterite said spec works fine.

So it's seems implementing this feature would have a huge impact on compilation time and the recommendation is to explicitly use private methods instead of hooks. Or use an alternative like minitest.

@bew
Copy link
Contributor

bew commented Oct 10, 2017

We've seen a lot of mis-understanding regarding Spec.*_each methods, what about renaming Spec.before_each to Spec.global_before_each (& after_each => global_after_each) ? This way it'll be more clear that this will register a block to run before/after all following specs, regardless of where (the context) it's called.

@straight-shoota
Copy link
Member

... or make it only callable from top level name space. But that's probably not as easy to accomplish.

@robacarp
Copy link
Contributor

The method living at the Module level instead of in the dsl for the describe block is a nice indicator that the logic exists for the global scope and not for the immediate scope. If a local set of _each methods were implemented, would it make sense to simply implement them as part of the spec DSL?

The code might look like this:

    Spec.before_each do
      puts "executed before every test in the suite"
    end

    describe "tests" do
      before_each do
        puts "executed before tests in this describe"
      end

      it "executes both before_each and Spec.before_each" do
        ...
      end
    end

    describe "more tests" do
      it "only executes Spec.before_each" do
        ...
      end
    end

@RX14
Copy link
Contributor

RX14 commented Oct 10, 2017

There's no local before_each because it's largely useless to implement them. We have no let (and we shouldn't add it), so they cannot easily pass state to the actual specs. What's the actual usecase which wouldn't be clearer and better implemented using wrapper methods with blocks?

@akzhan
Copy link
Contributor

akzhan commented Oct 10, 2017

Anybody should know, that great implementation of RSpec will be greatly appropriated.

And formatters etc.

Simply now core tea have no resources to do all right.

This is strongly IMHO!

@asterite
Copy link
Member

If someone wants to implement this, please do so and send a PR. It should support kind of like instance variables, similar to what you can do in rspec or spec2.cr or minitest, otherwise it's pretty pointless.

Good luck 😉

@robacarp
Copy link
Contributor

@RX14 / @asterite these two comments seem in conflict, but perhaps I'm missing some subtlety:

We have no let (and we shouldn't add it), so they cannot easily pass state to the actual specs.

It should support kind of like instance variables, similar to what you can do in ...

Am I missing something about this? I've never been much of an rspec fan, especially the old rspec monkeypatch pattern, so perhaps I'm missing something about what you're saying here because I'm not in the RSpec know.

@RX14
Copy link
Contributor

RX14 commented Nov 18, 2017

@robacarp yeah they contradict, the core team is composed of people, naturally we disagree quite often. I take the view that the current spec harness is simple, understandable, and doesn't need any more advanced features. The role of let/before/after can be provided by helper methods in your spec_helper.cr, or private helper methods local to one file. Asterite obviously feels it's better to provide those features in the spec harness itself.

I say they're features which make a single it not self-contained, and dislocate behavior between let/before/after which are spread throughout a file. This makes it really hard to understand, especially with a long file in a large spec suite which you didn't write, you might totally miss a before/after. Helper methods are explicitly called from the it block, meaning they're much harder to hide or get confused about. Go does the same as I described, IIRC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants