-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't Crash on Map Update #22
Conversation
test/support/test_schema.ex
Outdated
@@ -0,0 +1,76 @@ | |||
defmodule ChannelSpec.SocketTest.LotsOfRefsSchema do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest creating modules in-place in the tests instead of helper modules, like here: https://github.com/felt/channel_spec/blob/main/test/channel_spec/socket_test.exs#L51-L69
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 ! Any specific reason? I broke it out since it was 75+ lines of code and that seemed like a lot to put in a test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it easier to have the whole setup in the test itself in macro heavy code to both have the full picture and making sure unrelated code is not affecting the generated code.
It's a matter of preference, but macros don't always produce helpful stacktraces so I'd rather avoid sharing modules in tests, and at least have the certainty that I know exactly which test is causing issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. However, the nested nature of the setup for this test doesn't allow us to make nested modules with refs. Example:
test "foo", %{mod: mod} do
# I can define the top level name
defmodule :"#{mod}.LotsOfRefsSchema" do
# And I can create modules in this namespace
defmodule Base do
def schema() do
%{
type: :object,
properties: %{
# But I can't reference it here — #{mod} is not available
foo: %{"$ref": :"#{mod}.LotsOfRefsSchema.Foo"},
},
additionalProperties: false
}
end
end
defmodule Foo do
def schema() do
%{type: :object}
end
end
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error: undefined variable "mod"
test/channel_spec/socket_test.exs:613: Test776.LotsOfRefsSchema.Base.schema/0
** (CompileError) test/channel_spec/socket_test.exs: cannot compile module Test776.LotsOfRefsSchema.Base (errors have been logged)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm that's a good point. You could move the defmodule Foo
inside defmodule Base
and use __MODULE__.Foo
instead, or not nest them(you'd repeat .LotsofRefsSchema
quite a bit) but it's a good point still.
I'm fine with either approach seeing a helper module may be easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed a commit. I was able to get it to work with a @mod_base Module.split(__MODULE__) |> Enum.drop(-1) |> Module.concat()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice trick! 💜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change fixes the issue and the new test extensively covers the ref resolution functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the test! I have a small suggestion for cleaning up the module generation
test/channel_spec/socket_test.exs
Outdated
|
||
defmodule Base do | ||
@mod_base Module.split(__MODULE__) |> Enum.drop(-1) |> Module.concat() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defmodule Base do | |
@mod_base Module.split(__MODULE__) |> Enum.drop(-1) |> Module.concat() | |
mod_base = __MODULE__ | |
defmodule Base do | |
@mod_base mod_base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice — added in 3fe90cc
Why am I opening this PR?
This PR addresses an issue with resolving
"$ref"
. I'm not entirely sure how to reproduce to make a test case. It seems that in some cases,:__unresolved_refs
gets deleted, or maybe not generated at all, for a given schema.I was experiencing an issue in https://github.com/felt/felt/pull/12945 with
$ref
and made this change locally. The schema properly generates and validates with this change.