icy-arctic-fox
commented
1 year ago It appears that the mock code doesn't account for free variables in return types.
Closed postmodern closed 1 year ago
icy-arctic-fox
commented
1 year ago It appears that the mock code doesn't account for free variables in return types.
icy-arctic-fox
commented
1 year ago A fix for this issue and various related ones has been pushed to master. Can you confirm this fixes the issue with free variables in type annotations? I suspect there may be other issues porting chars.cr to the latest Spectator mocks. Please open an issue if you encounter any.
postmodern
commented
1 year ago @icy-arctic-fox awesome, that got me further. I now think I'm hitting another spectator bug.
Example code:
#
# Returns an Array of random, but unique, bytes from the `CharSet`.
#
def random_distinct_bytes(length : Int) : Array(Int32)
@byte_set.to_a.shuffle[0,length]
end
#
# Returns an Array of random, but unique, bytes from the `CharSet`.
#
def random_distinct_bytes(length : Array(Int),
random = Random::DEFAULT) : Array(Int32)
bytes.shuffle[0,length.sample(random)]
end
#
# Returns an Array of random, but unique, bytes from the `CharSet`.
#
def random_distinct_bytes(length : Range(Int, Int)) : Array(Int32)
@byte_set.to_a.shuffle[0,rand(length)]
endExample spec code:
describe "#random_distinct_bytes" do
it "must return a random Array of unique bytes" do
bytes = subject.random_distinct_bytes(10)
expect(bytes.uniq).to be == bytes
expect(bytes.all? { |b| subject.includes?(b) }).to be(true)
end
context "with a range of lengths" do
it "must return a random Array of unique bytes with a varying length" do
bytes = subject.random_distinct_bytes(5..10)
expect(bytes.uniq).to be == bytes
expect(bytes.size).to be_between(5, 10)
expect(bytes.all? { |b| subject.includes?(b) }).to be(true)
end
end
endError:
...
There was a problem expanding macro 'default_stub'
Called macro defined in lib/spectator/src/spectator/mocks/stubbable.cr:108:13
108 | private macro default_stub(method)
Which expanded to:
1 |
2 |
3 |
4 |
5 |
6 | def random_distinct_bytes(
7 | length : Range(Int, Int),
8 |
9 |
10 | ) : Array(Int32)
11 | super(length)
12 | end
13 |
14 |
15 |
16 |
17 | def random_distinct_bytes(
18 | length : Range(Int, Int),
19 |
20 |
21 | ) : Array(Int32)
22 |
23 | # Capture information about the call.
> 24 | __temp_1575 = ::Spectator::MethodCall.build(
25 | :random_distinct_bytes,
26 | ::NamedTuple.new(
27 | "length": length,
28 | ),
29 |
30 | ::NamedTuple.new(
31 |
32 | ).merge()
33 | )
34 | _spectator_record_call(__temp_1575)
35 |
36 | # Attempt to find a stub that satisfies the method call and arguments.
37 | # Finding a suitable stub is delegated to the type including the `Stubbable` module.
38 | if __temp_1576 = _spectator_find_stub(__temp_1575)
39 | # Cast the stub or return value to the expected type.
40 | # This is necessary to match the expected return type of the original method.
41 | _spectator_cast_stub_value(__temp_1576, __temp_1575, typeof(previous_def),
42 | :raise)
43 | else
44 | # Delegate missing stub behavior to concrete type.
45 | _spectator_stub_fallback(__temp_1575, typeof(previous_def)) do
46 | # Use the default response for the method.
47 | previous_def
48 | end
49 | end
50 | end
51 |
Error: instantiating 'Spectator::MethodCall.class#build(Symbol, NamedTuple(length: Range(Int32, Int32)), NamedTuple())'
In lib/spectator/src/spectator/mocks/method_call.cr:27:35
27 | arguments = FormalArguments.build(*args, **kwargs).as(AbstractArguments)
^----
Error: instantiating 'Spectator::FormalArguments(Args, Splat, DoubleSplat).class#build(Tuple(NamedTuple(length: Range(Int32, Int32)), NamedTuple()), NamedTuple())'
In lib/spectator/src/spectator/mocks/formal_arguments.cr:36:7
36 | new(args, nil, nil, kwargs)
^--
Error: instantiating 'new(NamedTuple(length: Range(Int32, Int32)), Nil, Nil, NamedTuple())'
Error: instantiating 'Spectator::AbstractArguments+#===(Spectator::AbstractArguments+)'
In lib/spectator/src/spectator/mocks/stub.cr:47:18
47 | constraint === call.arguments
^-
Error: instantiating 'Spectator::AbstractArguments+#===(Spectator::AbstractArguments+)'
In lib/spectator/src/spectator/mocks/formal_arguments.cr:130:7
130 | compare_named_tuples(args, other.args) && compare_tuples(splat, other.splat) && compare_named_tuples(kwargs, other.kwargs)
^-------------------
Error: instantiating 'compare_named_tuples(NamedTuple(length: Range(Int32, Int32)), (NamedTuple() | NamedTuple(byte: Int32 | UInt8) | NamedTuple(char: Char) | NamedTuple(io: String::Builder) | NamedTuple(length: Int32 | Range(Int32, Int32)) | NamedTuple(length: Int32, random: Random::PCG32) | NamedTuple(method: Symbol) | NamedTuple(n: Int32) | NamedTuple(random: Random::PCG32) | NamedTuple(value: Char)))'
In lib/spectator/src/spectator/mocks/abstract_arguments.cr:35:9
35 | a.each do |k, v1|
^---
Error: instantiating 'NamedTuple(length: Range(Int32, Int32))#each()'
In lib/spectator/src/spectator/mocks/abstract_arguments.cr:35:9
35 | a.each do |k, v1|
^---
Error: instantiating 'NamedTuple(length: Range(Int32, Int32))#each()'
In lib/spectator/src/spectator/mocks/abstract_arguments.cr:42:34
42 | return false unless v1 === v2
^-
Error: instantiating 'Range(Int32, Int32)#===((Char | Int32 | Random::PCG32 | Range(Int32, Int32) | String::Builder | Symbol | UInt8))'
In /var/lib/snapd/snap/crystal/1384/share/crystal/src/range.cr:327:5
327 | includes?(value)
^--------
Error: instantiating 'includes?((Char | Int32 | Random::PCG32 | Range(Int32, Int32) | String::Builder | Symbol | UInt8))'
In /var/lib/snapd/snap/crystal/1384/share/crystal/src/range.cr:298:35
298 | (begin_value.nil? || value >= begin_value) &&
^----------
Error: expected argument #1 to 'Char#>=' to be Char, not Int32
Overloads are:
- Char#>=(other : Char)
- Comparable(T)#>=(other : T)Pushed another change to master that should fix range comparisons.
The underlying issue is that a union of argument types are being compared against each other. Case-equality matching (===) is used. This creates all sorts of combinations like (in your case):
(1..10) === 'a'Which doesn't make sense, but could be a possible comparison with the union types. What I'm doing for now is using == for when === doesn't make sense. This is case-by-case. The problem with this is that any custom types defining a === method with no type restrictions may also hit this. I'm at least trying to cover the standard library types.
If you (or anyone else) has a better solution, I'd love to hear it!
postmodern
commented
1 year ago @icy-arctic-fox got a little further.
Crystal code:
@[AlwaysInline]
def +(other : CharSet) : CharSet
self | other
endCrystal spec:
describe "#+" do
mock Chars::CharSet do
stub :|, other : Chars::CharSet
end
let(other) { described_class.new }
let(return_value) { described_class.new }
it "must call #+" do
expect(subject).to receive(:|).with(other).and_return(return_value)
expect(subject + other).to be(return_value)
end
endError:
In spec/char_set_spec.cr:658:1
658 | it "must call #+" do
^
Error: expanding macro
There was a problem expanding macro 'it'
Called macro defined in macro 'define_example'
17 | macro it(what = nil, *tags, **metadata, &block)
Which expanded to:
> 1 |
> 2 |
> 3 |
> 4 | _spectator_metadata(__temp_46, :metadata, )
> 5 | _spectator_metadata(__temp_731, __temp_46, )
> 6 |
> 7 |
> 8 |
> 9 |
> 10 | private def __temp_732() : Nil
> 11 | (expect(subject)).to(((receive(:|)).with(other)).and_return(return_value))
> 12 | (expect(subject + other)).to(be(return_value))
> 13 |
> 14 | end
> 15 |
> 16 | ::Spectator::DSL::Builder.add_example(
> 17 | _spectator_example_name("must call #+"),
> 18 | ::Spectator::Location.new("/home/postmodern/code/crystal/chars.cr/spec/char_set_spec.cr", 658, 662),
> 19 | -> { new.as(::Spectator::Context) },
> 20 | __temp_731
> 21 | ) do |example|
> 22 | example.with_context(SpectatorTestContext::Group__temp_205::Group__temp_726) do
> 23 |
> 24 | __temp_732
> 25 |
> 26 | end
> 27 | end
> 28 |
> 29 |
> 30 |
Error: instantiating 'Spectator::Example#with_context(SpectatorTestContext::Group__temp_205::Group__temp_726.class)'
In spec/char_set_spec.cr:658:1
658 | it "must call #+" do
^
Error: expanding macro
There was a problem expanding macro 'it'
Called macro defined in macro 'define_example'
17 | macro it(what = nil, *tags, **metadata, &block)
Which expanded to:
> 1 |
> 2 |
> 3 |
> 4 | _spectator_metadata(__temp_46, :metadata, )
> 5 | _spectator_metadata(__temp_731, __temp_46, )
> 6 |
> 7 |
> 8 |
> 9 |
> 10 | private def __temp_732() : Nil
> 11 | (expect(subject)).to(((receive(:|)).with(other)).and_return(return_value))
> 12 | (expect(subject + other)).to(be(return_value))
> 13 |
> 14 | end
> 15 |
> 16 | ::Spectator::DSL::Builder.add_example(
> 17 | _spectator_example_name("must call #+"),
> 18 | ::Spectator::Location.new("/home/postmodern/code/crystal/chars.cr/spec/char_set_spec.cr", 658, 662),
> 19 | -> { new.as(::Spectator::Context) },
> 20 | __temp_731
> 21 | ) do |example|
> 22 | example.with_context(SpectatorTestContext::Group__temp_205::Group__temp_726) do
> 23 |
> 24 | __temp_732
> 25 |
> 26 | end
> 27 | end
> 28 |
> 29 |
> 30 |
Error: instantiating '__temp_732()'
In spec/char_set_spec.cr:659:24
659 | expect(subject).to receive(:|).with(other).and_return(return_value)
^-
Error: instantiating 'Spectator::Expectation::Target(Chars::CharSet)#to(Spectator::ValueStub(Chars::CharSet))'
In lib/spectator/src/spectator/expectation.cr:104:7
104 | def to(stub : Stub, message = nil) : Nil
^-
Error: The syntax `expect(...).to receive(...)` requires the expression passed to `expect` be stubbable (a mock or double)It looks like the old (v0.9) stub syntax is being used. A method definition (def) is required after the stub keyword. That should have raised a compiler error, so I'll look into that...
mock Chars::CharSet do
stub :|, other : Chars::CharSet
end
# can be just:
mock Chars::CharSetThe v0.11 syntax doesn't require a stub entry for each stubbed method. The mocked type also has to be instantiated with mock(Type).
I think this code should work:
Spectator.describe Chars::CharSet do
mock Chars::CharSet
describe "#|" do
let(other) { mock(Chars::CharSet) }
let(return_value) { mock(Chars::CharSet) }
it "must call #+" do
expect(subject).to receive(:|).with(other).and_return(return_value)
expect(subject | other).to be(return_value)
end
end
endMore info about the new syntax and mock system can be found on the wiki.
postmodern
commented
1 year ago Success! After adding the double mock statements it worked. I'm curious why are two mock statements required, in the specs and another within a let/subject?
icy-arctic-fox
commented
1 year ago I was attempting to copy the code you had before. This bit from the original:
let(other) { described_class.new }
let(return_value) { described_class.new }The mock outside of the it block is a macro that defines a sub-type which mocks the original. The mock inside of a test block (or any other function like let) instantiates the previously defined mock. These can be replaced with def_mock and new_mock respectively if it makes more sense.
Spectator.describe Chars::CharSet do
def_mock Chars::CharSet
let(other) { new_mock(Chars::CharSet) }
let(return_value) { new_mock(Chars::CharSet) }
endHope that makes sense. :confused:
I'm glad it works now! Real world usages of Spectator are invaluable for testing it.
I was trying to test chars.cr against Crystal 1.6.2 and Spectator 0.11.4, but got this strange error coming from deep within
enumerable.cr. The beginning of the error is a macro expansion of a method stub. I assume I need to update how I'm defining the stubs?Error Trace