Skip/fix tests on OTP 19 #4855
Skip/fix tests on OTP 19 #4855
Conversation
|
This doesn't look like "make tests pass" but rather "don't run tests that fail". Could we split this into a fix for callbacks and a commit that skips dialyzer tests? |
|
Sure.
|
ericentin
force-pushed
the
otp-19-test-fixes
branch
from
0a34132 to
27fe0c7
Compare
| @@ -71,6 +71,7 @@ defmodule IEx.HelpersTest do | |||
| cleanup_modules([Sample]) | |||
| end | |||
|
|
|||
| @tag :does_not_apply_to_otp19 | |||
There was a problem hiding this comment.
I think we need to make this test pass properly.
There was a problem hiding this comment.
The only issue is that on OTP 19 the module will be in memory and thus we cannot get the abstract code in the h helper, even if we address the issue in the way I described in the PR description.
There was a problem hiding this comment.
Oh, wait. I see that this test actually compiles the files to beams in this test. Should be possible to make it work then.
ericentin
force-pushed
the
otp-19-test-fixes
branch
from
27fe0c7 to
1bd0b16
Compare
| @@ -1,4 +1,10 @@ | |||
| ExUnit.start [trace: "--trace" in System.argv] | |||
| exclude = | |||
| case :erlang.system_info(:otp_release) do | |||
There was a problem hiding this comment.
I think we can do this:
otp_release = List.to_integer :erlang.system_info(:otp_release)
ExUnit.start [exclude: [:otp_release], include: [otp_release: otp_release], trace: "--trace" in System.argv]And for the tests that do not work on 19, we can do: @tag otp_release: 18.
What do you think?
There was a problem hiding this comment.
Will we continue to skip the tests if this get patched before OTP 20?
There was a problem hiding this comment.
Seems the functional difference would be that then tests will be disabled on versions before and after 18, vs. the current approach which only disables the tests on 19. I think we'd rather keep the tests running on previous versions (for anyone still developing on 17, unless that is no longer supported?) and ensure that the disabled tests run again on any new versions?
There was a problem hiding this comment.
We would have to manually remove the skip for 19.1 for instance, in any event.
There was a problem hiding this comment.
We wont be able to compile on 17 anymore so dont worry about that.
There was a problem hiding this comment.
For future cases like this I think we should stick to using application versions because people can build custom versions of OTP that don't match an OTP version.
There was a problem hiding this comment.
Yes, one step at a time. And this are tests, we can afford to be flexible and change later on.
OK lets stick with :otp_release I was just trying to drive home the point that this kind of version checking is often inadequate.
There was a problem hiding this comment.
Ok, so I'm gonna stick with it as is so that the tests start running again on OTP 20? Does that sound ok?
There was a problem hiding this comment.
I really don't like the current tag name though. Can we use a Boolean flag
for :otp19 then? And we exclude based on true and false.
On Thursday, June 23, 2016, Eric Entin [email protected] wrote:
In lib/elixir/test/elixir/test_helper.exs
#4855 (comment):@@ -1,4 +1,10 @@
-ExUnit.start [trace: "--trace" in System.argv]
+exclude =
- case :erlang.system_info(:otp_release) do
Ok, so I'm gonna stick with it as is so that the tests start running again
on OTP 20? Does that sound ok?—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/elixir-lang/elixir/pull/4855/files/1bd0b165454dd143d26c34444bdb599faa942b2a#r68151089,
or mute the thread
https://github.com/notifications/unsubscribe/AAAlbjt2iwzcU19WfKbQVrDCCB6H1CI4ks5qOb-3gaJpZM4I8OnO
.
José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D
There was a problem hiding this comment.
That's fine. I'm not married to the name. :)
|
Okey dokey, I think we're good to go now. I switched to using Additionally, I identified a potential bug (looks like |
|
❤️ 💚 💙 💛 💜 |
* Callback-related tests use beam AST to find callbacks * Fix invalid function call warning on OTP 19 * Skip dialyzer-related tests on OTP 19 * Fix IEx h helper for OTP 19 * Improve IEx h helper test for multi-arity callbacks Signed-off-by: José Valim <[email protected]>
Addresses #4851.
In addition to the original issues from #4851, a test for
IExrelated to thehhelper and callback functions was failing. I am currently skipping that test, but we can updatehto use a similar approach tocallbacks_for_beaminelixir/test_helper.exsas a fallback as part of this PR. That way it will continue to work for in memory and disk modules in <19, but it will only work for disk modules in >19.I've also posted to erlang-questions asking why callbacks are no longer in the attributes, since I cannot find any reference to such a change in the changelog for 19, the github, or the erlang issue tracker. It seems this may be an unexpected change?
/cc @josevalim