ihumanable / patch

Ergonomic Mocking for Elixir
MIT License
193 stars 11 forks source link

Silences unexpected message and fixes 1.16 deprecation #63

Closed hissssst closed 2 months ago

hissssst commented 6 months ago

Hey, thanks for this great library!

My testing logs are getting flooded by messages like

Error: 16:14:39.389 [error] Patch.Mock.Server Patch.Mock.Server.For.Some.Module received unexpected message in handle_info/2: {:EXIT, #PID<0.11558.0>, :normal}

And also

warning: negative steps are not supported in String.slice/2, pass 1..-2//1 instead
  (elixir 1.16.2) lib/string.ex:2369: String.slice/2
  (elixir 1.16.2) elixir_dispatch.erl:228: :elixir_dispatch.expand_macro_fun/7
  (elixir 1.16.2) elixir_dispatch.erl:215: :elixir_dispatch.expand_require/6
  (elixir 1.16.2) elixir_dispatch.erl:136: :elixir_dispatch.dispatch_require/7
  (elixir 1.16.2) elixir_expand.erl:599: :elixir_expand.expand_arg/3
  (elixir 1.16.2) elixir_expand.erl:615: :elixir_expand.mapfold/5
  (elixir 1.16.2) elixir_expand.erl:870: :elixir_expand.expand_remote/8
  (elixir 1.16.2) elixir_dispatch.erl:248: :elixir_dispatch.expand_quoted/7
  (elixir 1.16.2) elixir_expand.erl:599: :elixir_expand.expand_arg/3
  (elixir 1.16.2) elixir_bitstring.erl:146: :elixir_bitstring.expand_expr/5
  (elixir 1.16.2) elixir_bitstring.erl:34: :elixir_bitstring.expand/8
  (elixir 1.16.2) elixir_bitstring.erl:26: :elixir_bitstring.expand/5
  (elixir 1.16.2) elixir_expand.erl:10: :elixir_expand.expand/3
  (elixir 1.16.2) elixir_expand.erl:548: :elixir_expand.expand_block/5
  (elixir 1.16.2) elixir_expand.erl:46: :elixir_expand.expand/3
  (elixir 1.16.2) elixir_clauses.erl:45: :elixir_clauses.clause/6
  (elixir 1.16.2) elixir_clauses.erl:347: anonymous fn/7 in :elixir_clauses.expand_clauses_origin/6
  (stdlib 5.2.1) lists.erl:1706: :lists.mapfoldl_1/3

This PR overrides default handle_info into a handler which does nothing. And adds generated tag to all macros, plus fixes the deprecation itself

PS: Minor version bump and publish would be heavily appreciated.

ihumanable commented 6 months ago

The changes to the macros and the catch all handle_info are fine. The included fix for the 1.16 warning can not be included because that syntax is invalid in any version of Elixir prior to 1.12 when the range literal with step syntax was introduced. Since the library supports users down to Elixir 1.7 I wrote https://github.com/ihumanable/patch/pull/64 to address that particular concern in a backwards compatible way.

As an aside, no one should be sending the Mock.Server any messages, given the bug description it seems likely that the patch you are providing is establishing a monitor to a process which is being evaluated in the Mock.Server process context and that's why it's getting extraneous messages. It might be a good idea not to do that in the first place, but I have no objection to including the catch all if you remove the incompatible range literal code.

hissssst commented 6 months ago

remove the incompatible range literal code

Thanks for the explanation. Done.

ihumanable commented 2 months ago

Since this got addressed in https://github.com/ihumanable/patch/pull/64 I'm going to close out this PR.

hissssst commented 2 months ago

@ihumanable but what about the GenServer message silence?

ihumanable commented 2 months ago

@hissssst did you ever track down the cause of that? I'm sorta torn on whether or not suppressing those is a good idea.

The only code that should be calling the mock server is the mock delegate, extraneous messages to the mock server generally mean something is wrong with the test (most commonly uncontrolled lifecycle or accidental use of the mock server pid). The messages can serve the purpose of informing the test author that there's an underlying issue with the way they've arranged their test. Suppressing that feels a bit like hiding away important information for the end user and making it "seem" like everything is ok.

Can you provide me more information on what you found about where the messages were coming from? If there is a usage of patch that seems fine but produces nuisance logs I'd like to fix that.

FWIW, there's a larger architectural issue in patch about how mock values work, they get executed in the mock server, which makes it too easy to address the mock server directly. I have a plan on how to fix this, which is to move the execution of the mock values over to the delegate, which might be a better solution than suppressing errors in the mock server. Instead of making the mock server more tolerant of unexpected messages this would make the sending of messages to the mock server nearly impossible to do by accident and achieve the same result, no annoying logs.

hissssst commented 2 months ago

they get executed in the mock server, which makes it too easy to address the mock server directly

Yes, that's the reason. Try setting up a task or any process with trap exit in the patch and you'll have this problem

hissssst commented 3 weeks ago

@ihumanable , hey, I need this PR, otherwise I get a lot of log messages

ihumanable commented 2 weeks ago

@hissssst I saw that you closed this PR, I'm sorry it took a bit to fix this. Can you take a look at version 0.14.0, I've changed how mocks work to fix this issue. Mocks will actually execute in the proper context now.