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

Detect nesting it and pending at run-time #7297

Conversation

makenowjust
Copy link
Contributor

Fix #7192
Close #7207

Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @makenowjust 👍

src/spec/context.cr Outdated Show resolved Hide resolved
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Just asking: Is this really worth the added complexity?

@makenowjust
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

@Fryguy Fryguy Jan 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bcardiff bcardiff added this to the 0.27.1 milestone Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants