Resolve inconsistent use of #inspect in expect_raises #16265

andrykonchin · 2025-10-24T16:09:24Z

Changes:

use #inspect for both expected and actual message in the same way
changed format of reporting failures to have details for expected and actual exception on separate lines
use the same new format when expected class doesn't match actual class
added missing specs for #expect_raises

Example:

For the given test:

it do
  expect_raises(Exception, %q(a\tb\nc)) do
    raise %q(a\tb\nc).inspect
  end
end

Output is the following

Failures:

  1) expectations assert
     Failure/Error: expect_raises(Exception, %q(a\tb\nc)) do

       Expected Exception with message containing: "a\\tb\\nc"
            got Exception with message: "\"a\\\\tb\\\\nc\""

Closes #14030

andrykonchin · 2025-10-24T16:22:48Z

The failures caused by the change:

> - unless ex_to_s.includes?(message)
> + unless ex.message == message

I assume that that relaxing and matching only a substring was not intentional and the expected behaviour to check for equality. Does it make sense?

src/spec/expectations.cr

andrykonchin · 2025-10-24T17:19:26Z

Wondering if there is a reasonable way to check correctness of backtrace in an error message.

It seems a hardcoded # spec/std/spec/expectations_spec.cr:#{__LINE__ - 7}:13 in '->' doesn't work in the following cases:

Expected:   "Expected ArgumentError\n" +
       "     got IndexError with message: \"Index out of bounds\"\n" +
       "Backtrace:\n" +
       "  # spec\\std\\spec\\expectations_spec.cr:315 in '->'\n" +
       "  # src\\spec\\example.cr:50 in 'internal_run'\n" +
       "  # src\\spec\\example.cr:38 in 'run'\n" +

https://github.com/crystal-lang/crystal/actions/runs/18786062634/job/53605370730?pr=16265

  Expected:   "Expected ArgumentError\n" +
         "     got IndexError with message: \"Index out of bounds\"\n" +
         "Backtrace:\n" +
         "  # .build/std_spec in 'Exception::CallStack::unwind:Array(Pointer(Void))'\n" +
         "  # .build/std_spec in 'Exception::CallStack#initialize:Array(Pointer(Void))'\n" +
         "  # .build/std_spec in 'Exception::CallStack::new:Exception::CallStack'\n" +
         "  # .build/std_spec in 'raise<IndexError>:NoReturn'\n" +
         "  # .build/std_spec in '~procProc(Nil)'\n" +

https://github.com/crystal-lang/crystal/actions/runs/18786062643/job/53604507713?pr=16265

straight-shoota · 2025-10-24T18:26:18Z

I don't think we need to check the backtrace here. That's a task for more specialized specs (like call_stack_spec.cr).

spec/std/spec/expectations_spec.cr

straight-shoota

Great job on the extensive specs ❤️

src/spec/expectations.cr

spec/std/spec/expectations_spec.cr

andrykonchin · 2025-10-24T20:20:39Z

There are a lot of failed compiler tests. Wondering how they are related to the changes.

 6) Semantic: struct errors if invoking to_unsafe and got different type
     Failure/Error: assert_error <<-CRYSTAL, "invoked 'to_unsafe' to convert from Foo to Int32, but got Char"

       Expected Crystal::TypeException with message containing: "invoked 'to_unsafe' to convert from Foo to Int32, but got Char"
            got Crystal::TypeException with message: "instantiating 'LibFoo::Foo#x=(Foo)'"
       Backtrace:
         # src/compiler/crystal/semantic/ast.cr:6:7 in 'raise'
         # src/compiler/crystal/semantic/ast.cr:5:5 in 'raise'
         # src/compiler/crystal/semantic/call.cr:1131:7 in 'instantiate'
         # src/compiler/crystal/semantic/call.cr:308:5 in 'lookup_matches_in_type'
         # src/compiler/crystal/semantic/call.cr:257:3 in 'lookup_matches_in_type:search_in_parents:with_autocast'
         # src/compiler/crystal/semantic/call.cr:236:5 in 'lookup_matches_in'
         # src/compiler/crystal/semantic/call.cr:235:3 in 'lookup_matches_in:with_autocast'
         # src/compiler/crystal/semantic/call.cr:192:7 in 'lookup_matches_without_splat'
         # src/compiler/crystal/semantic/call.cr:127:17 in 'lookup_matches:with_autocast'
         # src/compiler/crystal/semantic/call.cr:116:5 in 'lookup_matches'
         # src/compiler/crystal/semantic/call.cr:93:15 in 'recalculate'
         # src/compiler/crystal/semantic/main_visitor.cr:1423:7 in 'recalculate_call'
         # src/compiler/crystal/semantic/main_visitor.cr:1402:7 in 'visit'
         # src/compiler/crystal/syntax/visitor.cr:27:12 in 'accept'
         # src/compiler/crystal/semantic/main_visitor.cr:688:11 in 'visit'
         # src/compiler/crystal/syntax/visitor.cr:27:12 in 'accept'
         # src/compiler/crystal/semantic/main_visitor.cr:6:7 in 'visit_main:process_finished_hooks:cleanup:visitor'
         # src/compiler/crystal/progress_tracker.cr:22:7 in 'semantic'
         # spec/spec_helper.cr:81:3 in 'semantic:warnings:wants_doc:flags'
         # spec/spec_helper.cr:59:3 in 'semantic'
         # spec/spec_helper.cr:55:1 in 'semantic:inject_primitives:flags'
         # spec/spec_helper.cr:163:5 in 'assert_error'
         # spec/compiler/semantic/c_struct_spec.cr:372:5 in '->'
         # src/spec/example.cr:50:13 in 'internal_run'
         # src/spec/example.cr:38:16 in 'run'
         # src/spec/context.cr:20:23 in 'internal_run'
         # src/spec/context.cr:364:14 in 'run'
         # src/spec/context.cr:20:23 in 'internal_run'
         # src/spec/context.cr:173:7 in 'run'
         # src/spec/dsl.cr:219:7 in 'execute_examples'
         # src/spec/dsl.cr:194:13 in '->'
         # src/crystal/at_exit_handlers.cr:18:17 in 'run'
         # src/crystal/main.cr:64:14 in 'exit'
         # src/crystal/main.cr:50:5 in 'main'
         # src/crystal/system/unix/main.cr:9:3 in 'main'
         # /usr/lib/dyld in 'start'

     # spec/compiler/semantic/c_struct_spec.cr:372

https://github.com/crystal-lang/crystal/actions/runs/18788933780/job/53614137106

Sija · 2025-10-24T21:36:26Z

@andrykonchin Maybe it's because you're now using ex.message instead of ex.to_s?

straight-shoota · 2025-10-24T21:50:19Z

Yeah. message might only contain partial information. Seems like we need to take the full to_s output into account.

andrykonchin · 2025-10-24T23:10:53Z

Right, indeed.

Wondering how important is this #to_s semantic is in expect_raises. Would it be a breaking change to replace it with exception.message (maybe in a separate PR)?

There is one minor issue with the #to_s semantic (besides being surprising and not documented) - if actual exception message is nil - then it will not be reflected in a failed test output as far as Exception.new(nil).to_s # => "".

Also does it sound good just to modify the spec helper assert_error (that fails in the compiler specs) and to check exception.to_s in it explicitly to not depend on expect_raises and its semantic? As far as I see assert_error is really compiler specific so names like assert_type_exception/assert_semantic_error look better to me.

So instead of

crystal/spec/spec_helper.cr

Lines 161 to 165 in 6a14b81

    
           def assert_error(str, message = nil, *, inject_primitives = false, flags = nil, file = __FILE__, line = __LINE__) 
        
             expect_raises TypeException, message, file, line do 
        
               semantic str, inject_primitives: inject_primitives, flags: flags 
        
             end 
        
           end

it might look like the following

def assert_error(str, message = nil, *, inject_primitives = false, flags = nil, file = __FILE__, line = __LINE__)
    semantic str, inject_primitives: inject_primitives, flags: flags
rescue e : TypeException
    e.to_s.should contain(message) if message
else
  fail "expected TypeException but nothing was raised"
end

straight-shoota · 2025-10-25T10:02:41Z

Would it be a breaking change to replace it with exception.message

Yes! We can't do that.

exception.message is just a part of the entire exception's description.

Maybe that's all a bit sloppy naming.
But the reason is that exceptions may have additional context information that gets stringified only on demand (by Exception#to_s). For example, there's no reason to build a huge string to describe a complicated error situation, if the exception is rescued anyway and the string would never be used.

Perhaps we can improve this somehow and make expectations more clear?
I suppose some of the confusion may come from expect_raises having a message parameter, which suggests it would be used to compare the exception's message property.

There is one minor issue with the #to_s semantic (besides being surprising and not documented) - if actual exception message is nil - then it will not be reflected in a failed test output as far as Exception.new(nil).to_s # => "".

Such an empty exception still wouldn't match any expected message. So that should be entirely fine.
What test would you expect to fail and doesn't?

Also does it sound good just to modify the spec helper assert_error (that fails in the compiler specs) and to check exception.to_s in it explicitly to not depend on expect_raises and its semantic?

I'm not sure where you're going with this? We cannot change the semantics of expect_raises. It's part of the public API and this behaviour is expected everywhere, not just in compiler specs.

andrykonchin · 2025-10-25T11:44:03Z

Done.

There is a single failure in "wasm32-test" job. Wondering how to deal with it properly.

EXITING: Attempting to raise:
expected Spec::AssertionFailed but nothing was raised (Spec::AssertionFailed)

Error: failed to run main module `wasm32_std_spec.wasm`

Caused by:
    0: failed to invoke command default
    1: wasm trap: wasm `unreachable` instruction executed
       wasm backtrace:
           0: 0xceb25 - wasm32_std_spec.wasm!*raise<Spec::AssertionFailed>:NoReturn
           1: 0xceaee - wasm32_std_spec.wasm!*fail<String, String, Int32>:NoReturn
           2: 0x3fddc7 - wasm32_std_spec.wasm!~procProc(Nil)@spec/std/spec/expectations_spec.cr:212
           3: 0x769858 - wasm32_std_spec.wasm!*Spec::Example#internal_run<Time::Span, Proc(Nil)>:(Array(Spec::Result) | Nil)
           4: 0x769506 - wasm32_std_spec.wasm!*Spec::Example#run:Nil
           5: 0x76e015 - wasm32_std_spec.wasm!*Spec::ExampleGroup@Spec::Context#internal_run:Nil
           6: 0x76dc82 - wasm32_std_spec.wasm!*Spec::ExampleGroup#run:Nil
           7: 0x76e050 - wasm32_std_spec.wasm!*Spec::ExampleGroup@Spec::Context#internal_run:Nil
           8: 0x76dc82 - wasm32_std_spec.wasm!*Spec::ExampleGroup#run:Nil
           9: 0x75b725 - wasm32_std_spec.wasm!*Spec::RootContext@Spec::Context#internal_run:Nil
          10: 0x75b434 - wasm32_std_spec.wasm!*Spec::RootContext#run:Nil
          11: 0x698697 - wasm32_std_spec.wasm!*Spec::CLI#execute_examples:Nil
          12: 0xb59aa - wasm32_std_spec.wasm!~procProc(Int32, (Exception | Nil), Nil)@src/spec/dsl.cr:186
          13: 0x71f4cf - wasm32_std_spec.wasm!*Crystal::AtExitHandlers::run<Int32, (Exception+ | Nil)>:Int32
          14: 0x53be00 - wasm32_std_spec.wasm!*Crystal::exit<Int32, (Exception+ | Nil)>:Int32
          15: 0x53bd8b - wasm32_std_spec.wasm!*Crystal::main<Int32, Pointer(Pointer(UInt8))>:Int32
          16: 0xb45ea - wasm32_std_spec.wasm!main
          17: 0xb466c - wasm32_std_spec.wasm!main
          18: 0xd86ef4 - wasm32_std_spec.wasm!__main_void
          19: 0xb461b - wasm32_std_spec.wasm!_start
       note: using the `WASMTIME_BACKTRACE_DETAILS=1` environment variable to may show more debugging information

https://github.com/crystal-lang/crystal/actions/runs/18802079919/job/53650943220?pr=16265

andrykonchin · 2025-10-25T13:42:14Z

Yes! We can't do that.

exception.message is just a part of the entire exception's description.

I understand.

Perhaps we can improve this somehow and make expectations more clear?
I suppose some of the confusion may come from expect_raises having a message parameter, which suggests it would be used to compare the exception's message property.

Yeah, indeed. Even if the #to_s semantic is documented - it's still may be confusing and the "message" parameter name makes it worse. I suppose renaming this parameter would be a breaking change too. Created #16268 to discuss adding a new spec matcher to not introduce breaking change.

Such an empty exception still wouldn't match any expected message. So that should be entirely fine.
What test would you expect to fail and doesn't?

Indeed, it doesn't lead to false positives or negatives. I am talking about "failed test output". Let's consider an example

expect_raises(Exception, "abc") do
  raise Exception.new(nil)
end

The output is the following:

    Failure/Error: expect_raises(Exception, "abc") do

       Expected Exception with message containing: "abc"
            got Exception with message: ""
       Backtrace:

My point is that the message: "" part is not accurate. It neither is a message nor has value "". The message is nil.

In general both partial matching and #to_s semantic seem confusing to me and partial matching may also be dangerous and lead to false negatives. Wondering if there are examples of other common test frameworks that have exception matchers with this semantic.

andrykonchin · 2025-10-26T00:51:06Z

Would it be a breaking change to replace it with exception.message

Yes! We can't do that.

I understand.

Hm, would it be a breaking change if it isn't documented?

If the only specs that rely on this behaviour is Crystal internal specs (e.g. the compiler specs) then we are free to make the changes.

andrykonchin added 2 commits October 24, 2025 19:18

Change output format of failed #expect_raises in specs

dc197fd

Add more specs for #expect_raises

f8fb47c

andrykonchin force-pushed the ak/resolve-inconsistency-in-expect-raise branch from dc82cfe to f8fb47c Compare October 24, 2025 16:19

straight-shoota reviewed Oct 24, 2025

View reviewed changes

src/spec/expectations.cr Outdated Show resolved Hide resolved

src/spec/expectations.cr Outdated Show resolved Hide resolved

src/spec/expectations.cr Outdated Show resolved Hide resolved

straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:specs labels Oct 24, 2025

andrykonchin added 3 commits October 24, 2025 19:34

Revert partial matching of expected and actual messages

31274c3

Extract backtrace formatting into a helper method

d445a92

Fail when actual message is nil and given non-nil expected message

d6c864d

Don't check backtrace items in specs

434e6e8

Sija reviewed Oct 24, 2025

View reviewed changes

spec/std/spec/expectations_spec.cr Outdated Show resolved Hide resolved

straight-shoota reviewed Oct 24, 2025

View reviewed changes

src/spec/expectations.cr Outdated Show resolved Hide resolved

spec/std/spec/expectations_spec.cr Outdated Show resolved Hide resolved

andrykonchin added 2 commits October 24, 2025 23:12

Simplified backtrace formatting and inline it back

5e49b71

Use fail instead of raise to fail a test

b0756ec

andrykonchin added 2 commits October 25, 2025 13:49

Rollback #to_s semantic

853f2d9

Add specs for the #to_s semantic

32c8f7b

andrykonchin mentioned this pull request Oct 25, 2025

Add another spec matcher for exceptions #16268

Open

andrykonchin requested a review from straight-shoota October 25, 2025 16:12

Uh oh!

Resolve inconsistent use of #inspect in expect_raises #16265

Are you sure you want to change the base?

Resolve inconsistent use of #inspect in expect_raises #16265

Conversation

andrykonchin commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrykonchin commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrykonchin commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

straight-shoota commented Oct 24, 2025

Uh oh!

Uh oh!

straight-shoota left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

andrykonchin commented Oct 24, 2025

Uh oh!

Sija commented Oct 24, 2025

Uh oh!

straight-shoota commented Oct 24, 2025

Uh oh!

andrykonchin commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

straight-shoota commented Oct 25, 2025

Uh oh!

andrykonchin commented Oct 25, 2025

Uh oh!

andrykonchin commented Oct 25, 2025

Uh oh!

andrykonchin commented Oct 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andrykonchin commented Oct 24, 2025 •

edited

Loading

andrykonchin commented Oct 24, 2025 •

edited

Loading

andrykonchin commented Oct 24, 2025 •

edited

Loading

andrykonchin commented Oct 24, 2025 •

edited

Loading