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

Make specs pending instead of failing in no multicast environments #9566

Merged
merged 2 commits into from
Jul 6, 2020

Conversation

jhass
Copy link
Member

@jhass jhass commented Jul 2, 2020

See #9508 / #9508 (comment) for history and context.

I'd still argue to include this because while it doesn't happen in one of our current build environments, it might in somebody else's. Also I think the spec feature is quite nice :)

SEO: UDPSocket using IPv6 joins and transmits to multicast groups / UDPSocket using IPv4 joins and transmits to multicast groups / setsockopt: Protocol not available (Socket::Error)

@jhass jhass mentioned this pull request Jul 2, 2020
5 tasks
@waj
Copy link
Member

waj commented Jul 2, 2020

Sorry, I'm confused. Is this patch still necessary after #9438?

@jhass
Copy link
Member Author

jhass commented Jul 2, 2020

Yeah, I suspected them to have the same reason initially, but given the IPv6 issues reproduce in some environments which this then does not, it looks like it's independent.

I don't know what settings the hosted GHA runners have to trigger this.

Either way, #9438 did not do anything about these.

@@ -47,6 +47,8 @@ module Spec
rescue ex : Spec::AssertionFailed
@parent.report(:fail, description, file, line, Time.monotonic - start, ex)
Spec.abort! if Spec.fail_fast?
rescue ex : Spec::ExamplePending
@parent.report(:pending, description, file, line, Time.monotonic - start)
Copy link
Contributor

Choose a reason for hiding this comment

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

ex.message is not being used anywhere, is that intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

For now yes.

While I felt it important to be able to specify a reason in the pending! call, making it clearer why that call is happening all of the sudden, we currently have no holistic approach to pending reasons. That is the pending method does not accept a reason in any form and there's no existing reporting format/infrastructure for this. I think pending should gain a way to collect reasons and then we should update the formatter to present them, but I felt introducing that holistic approach here to be a bit too much for what it wants to do, also because there's probably a lot more opinion on how that should be done.

@waj waj added this to the 1.0.0 milestone Jul 3, 2020
@waj waj merged commit 0929912 into crystal-lang:master Jul 6, 2020
@jhass jhass deleted the specs_no_multicast branch July 6, 2020 13:39
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.

4 participants