icy-arctic-fox / spectator

Feature-rich testing framework for Crystal inspired by RSpec.
https://gitlab.com/arctic-fox/spectator
MIT License
103 stars 5 forks source link

Better error when attempting to use string interpolation in it descriptions? #41

Closed matthewmcgarvey closed 2 years ago

matthewmcgarvey commented 2 years ago

If you attempt to do something like it "sets #{my_var} on object" you get an ugly compiler error like

$ crystal spec spec/i18next_plurals_spec.cr
Showing last frame. Use --error-trace for full trace.

There was a problem expanding macro '_spectator_metadata'

Code in macro 'it'

 4 | _spectator_metadata(__temp_36, :metadata,  )
     ^
Called macro defined in lib/spectator/src/spectator/dsl/metadata.cr:6:13

 6 | private macro _spectator_metadata(name, source, *tags, **metadata)

Which expanded to:

 > 1 | private def self.__temp_36
                        ^--------
Error: can't declare def dynamically

Would it be possible to have a more helpful error for this? You can check for string interpolation in macros by doing something like {% if string.is_a?(StringInterpolation) %}

icy-arctic-fox commented 2 years ago

How about something even better? It was planned to support string interpolation in example names at some point, so I went ahead and added it. I'm guessing you want to do something like this:

let(answer) { 42 }

it "the answer is #{answer}" do
  # ...
end

This is now possible. The string interpolation runs in the context of the example.

String interpolation is not allowed in group names/descriptions. Those don't have a context (example to run in). An custom error message has been added for that.

The changes have been pushed to master. If you're okay with this solution, I can cut a release shortly.

matthewmcgarvey commented 2 years ago

The original error was raised in context of using

[..].each do |item|
  it "...#{item}..." do
  end
end

We'll be swapping over to using sample as intended but how will the string interpolation work in that scenario?

icy-arctic-fox commented 2 years ago

Using each to define methods in Spectator doesn't work. I'm aware of this trick in RSpec, Ruby allows defining methods dynamically, but Crystal obviously doesn't. Spectator would attempt to generate code like this:

[..].each do |item|
  # Some prelude stuff.

  def the_actual_test
    # ...
  end
end

Which is where the error: can't declare def dynamically comes from.

In Spectator, there's two ways to achieve this: sample (and its variants) and macros ({% for ... %}).

With the new code, the example name is interpolated as expected: https://github.com/icy-arctic-fox/spectator/commit/c1841526d4f21fb9005b8176ffe82366c3fe0832

def self.various_strings
  %w[foo bar baz]
end

sample various_strings do |string|
  it "works with #{string}" do |example|
    expect(example.name).to eq("works with #{string}")
  end
end
Spec metadata
  supports string interpolation
  various_strings
    string: "foo"
      works with foo
    string: "bar"
      works with bar
    string: "baz"
      works with baz
SamantazFox commented 2 years ago

Quick summary of https://github.com/iv-org/invidious/pull/2646:

I have a Hash(String, Enum) object that is used as the source for function input/output. I.e:

enum MyEnum
  Value1
  Value2
  Value3
end

TESTS = {
  "in1" => MyEnum::Value1,
  "in2" => MyEnum::Value2,
  "in3" => MyEnum::Value3,
}

And the following code:

Spectator.describe "my_test" do
  TESTS.each do |in, out|
    it "returns the right output for '#{in}'" do
      expect(resolver(in)).to eq(out)
    end
  end
end

The code above yielded the error reported by @matthewmcgarvey in the OP.


As proposed, I used sample instead, with the latest commit from master:

Spectator.describe "my_test" do
  sample TESTS do |in, out|
    it "returns the right output for '#{in}'" do
      expect(resolver(in)).to eq(out)
    end
  end
end

And I got this error message:

In lib/spectator/src/spectator/dsl/groups.cr:202:5

 202 | define_iterative_group :sample
       ^
Error: Expected 1 argument for 'sample' block, but got 2

Finally, I tried the following:

Spectator.describe "my_test" do
  sample TESTS.keys do |in|
    it "returns the right output for '#{in}'" do
      expect(resolver(in)).to eq(TESTS[in])
    end
  end
end

And it seem to work. Nope, it doesn't:

It does work nicely, I forgot the fact that the shard.yml hasn't been updated on our repo.

icy-arctic-fox commented 2 years ago

sample blocks only support one argument. That has been changed in 4057089c20290c1a983a8e21d19663cd4dc486a8.

This syntax should work now:

sample TESTS do |in, out|
  # ...
end

Try it out if you could (latest code is on master). If it all looks good, I'll release a new version with these fixes in.

SamantazFox commented 2 years ago

Try it out if you could (latest code is on master). If it all looks good, I'll release a new version with these fixes in.

Finished in 2.87 milliseconds
52 examples, 0 failures
Randomized with seed: 92001

Works perfectly! Thanks for implementing the features <3

icy-arctic-fox commented 2 years ago

Release v0.10.4 is out with all the changes. Let me know if there's anything else I can help with.

gmartsenkov commented 2 years ago

This seems to break the use of a normal array in sample

    sample [1,2,3] do |i|
      it "is itself" do
        expect(i).to eq i
      end
    end

There was a problem expanding macro 'let'

Code in macro 'sample'

 25 | let(i) do |example|
      ^
Called macro defined in lib/spectator/src/spectator/dsl/memoize.cr:11:5

 11 | macro let(name, &block)

Which expanded to:

   11 |         
   12 |         @__temp_85.get do
 > 13 |           (example.group.as(::Spectator::ExampleGroupIteration(typeof(Group__temp_81.__temp_82.first)))).item[0]
                                                                                                                     ^
Error: undefined method '[]' for Int32
icy-arctic-fox commented 2 years ago

This seems to break the use of a normal array in sample

Pushed a fix to master, can you try it out and verify it works for your use-case?

gmartsenkov commented 2 years ago

@icy-arctic-fox it works!

icy-arctic-fox commented 2 years ago

@gmartsenkov Cut a release (v0.10.5) to include the fix.