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

Detect nesting `it` and `pending` at run-time #7297

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@MakeNowJust
Copy link
Contributor

MakeNowJust commented Jan 11, 2019

Fix #7192
Close #7207

@sdogruyol
Copy link
Member

sdogruyol left a comment

Thank you @MakeNowJust 👍

Show resolved Hide resolved src/spec/context.cr Outdated
@RX14

RX14 approved these changes Jan 11, 2019

@asterite
Copy link
Contributor

asterite left a comment

Looks good! I just left a question.

@@ -385,6 +385,11 @@ module Spec
raise ex
end

# `NestingSpecError` is also treated.
if ex.is_a?(Spec::NestingSpecError) && klass != Spec::NestingSpecError

This comment has been minimized.

@asterite

asterite Jan 11, 2019

Contributor

What is this for?

This comment has been minimized.

@MakeNowJust

MakeNowJust Jan 11, 2019

Contributor

See full code:

    def expect_raises(klass : T.class, message = nil, file = __FILE__, line = __LINE__) forall T
      yield
    rescue ex : T
      # We usually bubble Spec::AssertaionFailed, unless this is the expected exception
      if ex.is_a?(Spec::AssertionFailed) && klass != Spec::AssertionFailed
        raise ex
      end

      # `NestingSpecError` is treated as the same above.
      if ex.is_a?(Spec::NestingSpecError) && klass != Spec::NestingSpecError
        raise ex
      end

      # ...
    end

Please tell me more correct sentence which means this if you know.

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Jan 11, 2019

Just asking: Is this really worth the added complexity?

@MakeNowJust

This comment has been minimized.

Copy link
Contributor

MakeNowJust commented Jan 11, 2019

Please don't merge this for now because this doesn't fix #7192 entirely. Sorry 🙇 I'll fix it asap.
Fixed!!

@straight-shoota At least we have an issue #7192 and this is the simplest way to fix it.

MakeNowJust added some commits Jan 11, 2019

Fix #7192 correctly
Currently nesting `it`/`pending` doesn't break `crystal spec -v` output.
@MakeNowJust

This comment has been minimized.

Copy link
Contributor

MakeNowJust commented Jan 11, 2019

Now, nesting pending/it doesn't break crystal spec -v output. I think it means #7192 is fixed.

Example:

require "spec"

it "foo" do
  it "bar" do
  end
end

it "baz" do
  pending
end

Old (broken) output:

2019-01-12 1 28 02

New (fixed) output:

2019-01-12 1 28 45

@@ -166,6 +166,19 @@ module Spec
def matches?(pattern, line, locations)
false
end

@@spec_nesting = false

This comment has been minimized.

@Fryguy

Fryguy Jan 18, 2019

Contributor

If we were to ever get to parallel test execution, would this class variable cause a problem?

This comment has been minimized.

@asterite

asterite Jan 18, 2019

Contributor

Yes

This comment has been minimized.

@asterite

asterite Jan 18, 2019

Contributor

Oh, actually, I was replying generically that class variables are to be used carefully in a parallel context. Not sure about this case, though

@RX14

RX14 approved these changes Jan 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment