andrewtimberlake / sham

An Elixir mock HTTP(S) server useful for testing HTTP(S) clients.
MIT License
3 stars 1 forks source link

Make `Sham.t` not be opaque #2

Closed warmwaffles closed 4 days ago

warmwaffles commented 6 days ago

Ran into the following issue:

Attempted to pattern match against the internal structure of an opaque term.

Type: Sham.t()

Pattern: %{:port => _port}

This breaks the opaqueness of the term.

This happens in some setup code where I need to do the following:

setup do
  sham = Sham.start()
  original = Application.get_env(:my_app, TheModule)
  Application.put_env(:my_app, TheModule, url: "http://localhost:#{sham.port}")
  on_exit(fn -> Application.put_env(:my_app, TheModule, original) end)
  {:ok, %{sham: sham}}
end

After changing from an opaque type to a normal type, it goes away.

andrewtimberlake commented 6 days ago

I’m not able to reproduce the problem. If I paste your setup code directly into one of my tests, it runs fine. What version of Elixir/OTP are you using?

warmwaffles commented 5 days ago

Did you put it in your MyApp.DataCase?

My dialyzer configs

    [
      plt_add_apps: [:mix, :ex_unit],
      flags: ~w(
        error_handling
        extra_return
        missing_return
      )a,
      plt_file: {:no_warn, "priv/plts/dialyzer-#{env}.plt"},
      list_unused_filters: true
    ]

Make sure to also run the dialyzer in the test env, not just a naked mix dialyzer but instead MIX_ENV=test mix dialyzer

Dialyzer won't check exs files. Sorry I should have specified that.

warmwaffles commented 5 days ago

@andrewtimberlake do you know why it was marked as opaque to begin with when the docs say to use a field sham.port on the opaque type?

https://hexdocs.pm/elixir/typespecs.html#user-defined-types

An opaque type, defined with @opaque is a type where the internal structure of the type will not be visible, but the type is still public.

warmwaffles commented 5 days ago

Here's a repo where it is reproduced.

https://github.com/warmwaffles/demo-sham-bug

Tool versions used

erlang 26.2.2
elixir 1.16.2-otp-26

But, when I bump to erlang 27.0.1 and elixir 1.17.2-otp-27 this appears to go away.

andrewtimberlake commented 5 days ago

I think it was a misunderstanding of opaque type on my part. My intention was that the %Sham{} type shouldn’t be created outside of Sham.start/1 What about using this, so that it’s explicit that the port should always be set, and that is the only part of the struct that is expected to be accessed?

 @type t :: %__MODULE__{
          port: pos_integer()
        }
warmwaffles commented 4 days ago

Yea that seems reasonable. I chucked nil in there as a possibility because of the defstruct pid: nil, port: nil.

andrewtimberlake commented 4 days ago

I’m not an expert on types. If you remove pid from the typespec, will dializer complain? My intention was to only expose the public API via the typespec, but if that’s not feasible, the I don’t mind.

warmwaffles commented 4 days ago

Well the struct module is exposed, so pid would be typed as term(). So it'll be present regardless. I removed the pid definition as being a pid() but the attr is still visible because it is a struct.

Edit: Did not receive an error, so leaving it as term is okay

andrewtimberlake commented 4 days ago

Thanks for working through this with me!

warmwaffles commented 4 days ago

No problem

warmwaffles commented 12 hours ago

Do you think we can get this cut as a release for 1.1.1 or 1.2.0 from the main branch? I'd like to stop relying on git in my deps.