-
-
Notifications
You must be signed in to change notification settings - Fork 197
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
Update Dependencies and test on Elixir 1.14 and 1.15 #738
Conversation
I opened up a PR on my fork so I could run CI and it seems to be failing for the same reason as the Selenium v4 macOS pipeline. It is pulling a newer version of elixir and there are deps that are incompatible with elixir 1.14+ |
.github/workflows/ci.yml
Outdated
elixir-otp: | ||
- otp: 25.x | ||
elixir: 1.14.x | ||
- otp: 26.x | ||
elixir: 1.15.x |
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.
why did you remove 1.12 and 1.13?
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.
Because they are not backwards compatible with the changed dependencies.
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.
This will likely require a major version bump to signify this change.
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 don't want to drop support for older versions of Elixir just for a dev dependency that I have never even used since taking over the project.
you can probably add --only test
to mix deps.get in CI to avoid having to update benchee.
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 am down to drop benchee as a dependency altogether. However there are other changes around ExUnit.Server.modules_loaded()
that are not backwards compatible.
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 is a private elixir API, and is in a unit test.
I would just add a conditional that checks if the function is exported and call the one that is
function_exported?(ExUnit.Server, :modules_loaded, 0)
, etc
@@ -73,7 +73,7 @@ defmodule Wallaby.HTTPClientTest do | |||
|
|||
test "includes the original HTTPoison error when there is one", %{bypass: bypass} do | |||
expected_message = | |||
"Wallaby had an internal issue with HTTPoison:\n%HTTPoison.Error{id: nil, reason: :econnrefused}" | |||
"Wallaby had an internal issue with HTTPoison:\n%HTTPoison.Error{reason: :econnrefused, id: nil}" |
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.
if this is flaky, i would do multiple =~
asserts instead
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'm not confident that it is flaky just yet. I want to understand why this change was necessary for this pass.
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.
otp changed how maps work for keys < 32 so now key order is not deterministic like it used to be
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.
Agreed. But if maps < 32 keys are treated like keyword lists in memory, they are linked and therefore have a deterministic order yes? So my inference would be that {:id, nil}
was added before {:reason, :econnrefused}
in the internals of HTTPoison, thereby putting :reason
at the head of the linked list.
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'm trying to say that in OTP26 that is no longer the case. the order is nondeterministic
https://www.erlang.org/blog/otp-26-highlights/#changed-ordering-of-atom-keys
You can pass custom_options: [sort_maps: true]
to as an Inspect.Opt tho, which will sort it and it should be deterministic.
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.
Ah. I see! Thanks for explaining 😃 I'll make some more adjustments and see if I can get things backwards compatible as well. Thanks for the feedback!
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.
Of course 😁
No description provided.