Code.compile_string/2 seems to interfere with other ExUnit tests

whitfin / local-cluster

Easy local cluster creation for Elixir to aid in unit testing

MIT License

223 stars 30 forks source link

Code.compile_string/2 seems to interfere with other ExUnit tests #16

Closed pmenhart closed 4 years ago

pmenhart commented 4 years ago

Using LocalCluster.start_nodes/3 with option :files in an umbrella project corrupted the ExUnit test management. The test file was re-compiled and re-executed for each subsequent umbrella app. A warning was emitted for each re-compile: warning: redefining module XXX (current version defined in memory)
This change avoids code changes on the master node by moving the compilation to each slave node (assuming the compiler is loaded there).

Note that this PR changes implementation for #8. Will it work for you? Is there a better solution? Please advise.

pmenhart commented 4 years ago

The Travis CI build fails only for Elixir 1.2. I'll wait for feedback before trying to fix.

whitfin commented 4 years ago

Seems fine to me, but the 1.2 build is curious - any ideas why?

whitfin commented 4 years ago

On second look, there's actually no issue here - the warning is just because the string is being compiled repeatedly. This is not an issue though because you will already have loaded the same module anyway, so the warning is redundant. I don't think we need to "fix" this. The warning is annoying, but it shouldn't be harming anything.

I have pushed https://github.com/whitfin/local-cluster/commit/62ce68aa65570d9153332e591303a7d5c578b096 to clean up this warning; if you think we need to do more please let me know :)

pmenhart commented 4 years ago

@whitfin Bad news: Code.compile_string/2 seems to be more sinister than just warnings! I tried the latest release, and our umbrella test suite is now executing LibCluster tests in an endless loop. Reproducibly. Individual tests are fine. I disabled ignore_module_conflict to see warnings: the module is compiled and executed repeatedly, after ExUnit moved to next app in the umbrella.

This problem is gone when I replace Code.compile_string/2 with rpc.(Code, :require_file, [file]) from this PR.

My suggestion: accept the PR, drop support for Elixir 1.2. (I am trying to find reason require_file is failing in 1.2, no clue yet. If you prefer to keep 1.2 support, we can make the code conditional based on Elixir version. Nasty solution, but it would work.)

whitfin commented 4 years ago

Are you including your test files? That's the only thing I can imagine, and that seems quite odd.

I'll take another look at this today and push a patch; let me think about 1.2 for a while.

pmenhart commented 4 years ago

Are you including your test files? Yes: we are sending anonymous functions between nodes, hence they need the compiled test script: LocalCluster.start_nodes("test-cluster", 2, [files: [__ENV__.file]]).

whitfin commented 4 years ago

@pmenhart ok after reviewing this I'm going to accept your change; I have no idea why v1.2 fails - it's something to do with the anonymous functions but I don't see much difference between the stdlib code for v1.2 and v1.3 here, so it feels like just a limitation of v1.2 somehow.

v1.2 still works with this lib, it's just that the anonymous functions do not. I don't think this is a major issue as there are ways around it and very few people are using v1.2 anyway, so I'm just going to merge as is.