Skip to content
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

Extend test matrix to cover supported versions #5681

Merged
merged 4 commits into from
Dec 29, 2023

Conversation

PragTob
Copy link
Contributor

@PragTob PragTob commented Dec 28, 2023

Not sure about the testing CI/strategy but I tried to extend it to cover all supported versions :)

  • Test against newest elixir & otp
  • Test against oldest supported elixir (Changelog and mix say 1.11) and I think we should test against it to avoid accidental brekage
  • updated patch versions for elixir versions

Thanks as always for your excellent work! 💚

Not sure about the testing CI/strategy but I tried to extend it to
cover all supported versions :)

* Test against newest elixir & otp
* Test against oldest supported elixir (Changelog and mix say 1.11)
  and I think we should test against it to avoid accidental brekage
* updated patch versions for elixir versions
@PragTob
Copy link
Contributor Author

PragTob commented Dec 28, 2023

I know there's also an integration test down there - should that also run on the most recent version or multiple? (maybe lowest and highest or keep as is)

@PragTob
Copy link
Contributor Author

PragTob commented Dec 28, 2023

Test break because a 1.12.0 feature is used:

Error: ** (CompileError) lib/phoenix/endpoint/cowboy2_adapter.ex:144: undefined function then/2
    (elixir 1.11.4) src/elixir_locals.erl:114: anonymous fn/3 in :elixir_locals.ensure_no_undefined_local/3
    (stdlib 3.8.2.4) erl_eval.erl:680: :erl_eval.do_apply/6

Should we:

  • rewrite for 1.11 compatibility
  • bump version requirement?

@josevalim
Copy link
Member

Great find @PragTob, let's rewrite it for compatibility! If further issues show up, then we bump it.

@PragTob
Copy link
Contributor Author

PragTob commented Dec 29, 2023

✔️ Get the task from José to remove all occurences of then/2 from the phoenix code base - life goal accomplished! (We don't need to tell anyone about it being due to the version breakage :P)

@PragTob
Copy link
Contributor Author

PragTob commented Dec 29, 2023

   1) test flash is persisted when status is a redirect (Phoenix.Controller.FlashTest)
Error:      test/phoenix/controller/flash_test.exs:42
     ** (UndefinedFunctionError) function :crypto.mac/4 is undefined or private. Did you mean one of:
     
           * cmac/3
           * cmac/4
           * hmac/3
           * hmac/4
           * hmac_init/2

That didn't fail locally - which is due to me using a newer OTP. I don't think rewriting those is worth it/don't wanna do it. I'll bump CI to OTP 22 and will try to document the version requirement.

The used `:crypto.mac` API was introduced in OTP 22.1
https://www.erlang.org/doc/man/crypto#mac-4

* bumped CI for 1.11 to 22.3 and so that 22.3 isn't run twice,
  bumped 1.12 to 23.3
* Documented in Changelog as I saw no other place
@PragTob
Copy link
Contributor Author

PragTob commented Dec 29, 2023

No idea why these may fail on 1.16:


  1) test with 1.0.0 serializer Phoenix.Socket.V1.JSONSerializer filter params on join (Phoenix.Integration.LongPollChannelsTest)
Error:      test/phoenix/integration/long_poll_channels_test.exs:608
     Assertion with =~ failed
     code:  assert log =~ "Parameters: %{\"foo\" => \"bar\", \"password\" => \"[FILTERED]\"}"
     left:  ""
     right: "Parameters: %{\"foo\" => \"bar\", \"password\" => \"[FILTERED]\"}"
     stacktrace:
       test/phoenix/integration/long_poll_channels_test.exs:622: (test)

.......................

  2) test with 2.0.0 serializer Phoenix.Socket.V2.JSONSerializer filter params on join (Phoenix.Integration.LongPollChannelsTest)
Error:      test/phoenix/integration/long_poll_channels_test.exs:608
     Assertion with =~ failed
     code:  assert log =~ "Parameters: %{\"foo\" => \"bar\", \"password\" => \"[FILTERED]\"}"
     left:  ""
     right: "Parameters: %{\"foo\" => \"bar\", \"password\" => \"[FILTERED]\"}"
     stacktrace:
       test/phoenix/integration/long_poll_channels_test.exs:622: (test)

@josevalim
Copy link
Member

@PragTob coincidentally Elixir 1.12 requires Erlang/OTP 22, so let's just bump to Elixir 1.12.

Separate commit in case it's unwanted to have here/it's also
a separate addition but I think it'd be nice to have in the
README.
@josevalim
Copy link
Member

josevalim commented Dec 29, 2023

@PragTob those are race conditions on log capture. it should be fine otherwise.

@PragTob
Copy link
Contributor Author

PragTob commented Dec 29, 2023

@josevalim yeah the test seems flakey :(

About bumping, some questions/clarifications:

  • do we really want to bump, bumping requirements on a patch level seems not great but also I don't think anyone would be affected 😅
  • the comment above the elixir version requirement says to bump the installer/make sure it's bumped - not sure where that is 😅

Thanks!

@josevalim
Copy link
Member

do we really want to bump, bumping requirements on a patch level seems not great

Hrm, thanks for double checking. I thought it was broken from v1.7.0 but only v1.7.10. So let's try to preserve it indeed.

the comment above the elixir version requirement says to bump the installer/make sure it's bumped - not sure where that is 😅

The installer is already on v1.14 or v1.15, so nothing to do in this case. The comment is when we bump the Phoenix requirement above the installer.

@josevalim
Copy link
Member

@PragTob the PR looks good to me if you are happy with it. :)

@PragTob
Copy link
Contributor Author

PragTob commented Dec 29, 2023

@josevalim I'm happy with it, thanks! 😁

@josevalim josevalim merged commit 5c768de into phoenixframework:main Dec 29, 2023
6 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@PragTob PragTob deleted the ci++ branch December 29, 2023 09:30
studzien pushed a commit to Whatnot-Inc/phoenix that referenced this pull request Feb 8, 2024
* Test against newest elixir & otp
* Test against oldest supported elixir (Changelog and mix say 1.11)
* Updated patch versions for elixir versions
* Fix accidental regression breaking elixir 1.11 support (then/2)
studzien pushed a commit to Whatnot-Inc/phoenix that referenced this pull request Feb 16, 2024
* Test against newest elixir & otp
* Test against oldest supported elixir (Changelog and mix say 1.11)
* Updated patch versions for elixir versions
* Fix accidental regression breaking elixir 1.11 support (then/2)
studzien pushed a commit to Whatnot-Inc/phoenix that referenced this pull request Feb 20, 2024
* Test against newest elixir & otp
* Test against oldest supported elixir (Changelog and mix say 1.11)
* Updated patch versions for elixir versions
* Fix accidental regression breaking elixir 1.11 support (then/2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants