Problem with assert_receive in koans

[iamvery]

I sort of stumbled upon the fact that assert_receive isn't working as expected in the koans. It's proven by this typo. I'm not yet sure what the problem is, but I'm assuming it's something to do with the various metaprogramming that's happening amongst these macros... I'll keep working on it, but any input is welcome!

[iamvery]

After a little more investigation, I'm leaning towards this being a bug in ExUnit. I'm going to take a little more time to verify I understand how assert_receive is designed to work, because TIL 😅

[iamvery]

Ok, here's what I know. The way ExUnit's assert_receive is implemented, the expression provided to the macro is used as a match for the receive statement expanded by the macro. So a variable used in the match would simply be overwritten unless it's pinned. That is exactly what's happening to the internal answer variable that's generated by the Koans macros.

Add the pin works, adds a little more cognitive load to the test cases.

How would we like to proceed? Is the pin not a big deal? Should we consider just writing the receive out instead of using assert receive? Are there any other options?

[iamvery]

It seems having assert_receive "leak" bindings into the calling code is an explicit feature of the macro. I still find it extremely surprising, but I suppose this is just a gotcha... elixir-lang/elixir@dbb6f35

[ukutaht]

Oh boy... this is hard.

Explicitly pinning the blanks (^__) seems like the most 'correct' way to deal with this but at the same time I feel like this will lead to confusion for people going through the koans. It's a tiny detail in the inner workings of ExUnit.. not sure I'd want to burden learners with that.

Writing the receive blocks out would work but again, it's longer and takes away the opportunity to teach users about assert_receive.

I wonder if there's something we can do behind the scenes to make our tests work better with assert_receive. Maybe the koan macro could pin the answer variable if the answer is within assert_receive? Seems odd, hard-to-write code but I'd rather take the hit on the tests side instead of making the user experience less clear.

After all if we don't come up with a good alternative, I'm happy with making some changes on the user side but I'd like to avoid it if at all possible.

Thank you for uncovering and investigating this issue by the way 👍 Had no idea this was even a problem.

[iamvery]

Great evaluation of the problem. I thought about trying to pin answer in koans. I like this option, because the actual solutions wouldn't require the pin. I'll take a shot at it and push things here.

[iamvery]

I just pushed a solution. TBH, I'm not thrilled with it. It seems hard to grok, BUT it does work. Let me know what you think :)

[iamvery]

I've got another idea. Instead of adding edge cases for Blanks, maybe we can just shim assert_receieve to add the pin for blanks. I'll give it a shot in a bit for comparison.

[iamvery]

Alright here's the alternative implementation: master...iamvery:assert-receive-alternative-fix

Basically instead of having custom replacements for assert_receive, this adds a small shim for that macro which ensures answer is pinned. I think my only concern with this approach is that if we're not careful, we might break someone's expectation for how that macro should behave. Not quite sure how to feel about that right now... Seems like both options have their tradeoffs.

[felipesere]

Amazing research! I remember having issues with assert_receive in the past. I don't think I came up with a decent solution as I didn't fully understand the problem/assert_receive.

I'll take a look tonight.

[felipesere]

I've had a brief look.
My personal preference is the variation with pre_pin/1. I was wondering though if we can ignore all of the pre-pinning and just have a special case of :assert_receive in pre/2? I there any benefit to the second Macro.prewalk?

[iamvery]

I might have been overthinking it, but the reasoning was that there is nothing to replace when :assert_receive is matched, it's the arguments to that function. First time using prewalk, so I may not have a clear understanding..

[iamvery]

Also fwiw, I really don't like the name of pre_pin, it was just the first thing that came to mind for "like the pre function except with pinning".

[ukutaht]

I agree with everything that's been said here. I think the pre_pin approach is the lesser of all evils we've considered and we should get this in.

@iamvery @felipesere Any objections to hitting merge on this?

[iamvery]

I just really wish it was cleaner. I'm maybe not completely certain all edge cases are covered.

[iamvery]

However probably no reason to hold it up. No ideas coming to mind...

[ukutaht]

I think the edge cases are not that big of a deal given that users never actually interact with this code. Any future problems that may crop up will affect us but users will be able to go through the koans just fine. That makes me a bit less concerned.

As for cleanliness.. I kind of accept some of it because we opted for more complex test code in order to leave the user experience intact. It seems that we don't have any ideas on how to make it cleaner and until we do I'm okay using the code as it is. In the end we gain the ability to properly test koans with assert_receive for the price of ~10 extra lines of code. Not too bad IMO.

[iamvery]