Extend PropEr with parallel execution #263
Conversation
Codecov Report
@@ Coverage Diff @@
## master #263 +/- ##
==========================================
+ Coverage 88.65% 88.95% +0.30%
==========================================
Files 14 14
Lines 4408 4601 +193
==========================================
+ Hits 3908 4093 +185
- Misses 500 508 +8
Continue to review full report at Codecov.
|
pablocostass
force-pushed
the
parallel_proper
branch
from
6bf55fe
to
c85d408
Compare
Before GitHub actions, we separated some lengthy (and flaky) examples tests out of `proper_tests` and placed them in a separate test target in the `Makefile`. For increased coverage, some of the examples were still tested in `proper_tests` as whole modules. Both for consistency and in preparation for using the examples as tests for #263, we now have a _single_ place (GitHub action) where the complete set of examples is tested.
pablocostass
force-pushed
the
parallel_proper
branch
from
c85d408
to
e057d79
Compare
pablocostass
force-pushed
the
parallel_proper
branch
from
e057d79
to
03937b3
Compare
|
@kostis I had forgotten to add the new make target to the Github Action workflow, so I did that after rebasing on top of the latest changes on the main branch. I see that the Github Action is still failing due to the cache, however this time it is because the current cache is from the main branch and in said branch we do not build the PLT file with |
|
I've pushed a small change to the Makefile that subsumes the addition of In order to achieve some progress, perhaps it's a good idea to temporarily disable the |
pablocostass
force-pushed
the
parallel_proper
branch
from
03937b3
to
1b8de04
Compare
|
Yup, that commit of yours helped @kostis, thanks! I rebased and pushed with the addition of a commit that disables the |
Codecov Report
@@ Coverage Diff @@
## master #263 +/- ##
==========================================
- Coverage 88.47% 85.17% -3.31%
==========================================
Files 14 14
Lines 4408 4586 +178
==========================================
+ Hits 3900 3906 +6
- Misses 508 680 +172
Continue to review full report at Codecov.
|
Modify the `user_opts` type and `opts` record to allow the possibility of spawning multiple processes to perform the tests Update `inner_test` function: each process is assigned a cerain number of tests to run from the total count. The main process aggregates the output received in case of success, otherwise returns early with the error or fail Some type specs were updated to reflect the changes needed to use the `spawn_link_migrate` function
This way, when using workers to distribute tests among processes, PropEr will first create a node for them to be spawned on, avoiding crashing the whole VM if something goes wrong.
…ach instead of lists:map
…sions When testing stateful programs, the node where the tests were being performed would crash because normally stateful programs use gen_servers or unique elements, which made processes modify each other's states. To avoid that now each process is spawned on a different node, which makes using many processes a bit more expensive.
As previously stated, for stateful properties each worker would need a node of his own to avoid clashing with the other workers, whereas on stateless properties all the workers could be performing tests on the same node. This commit fixes last one by starting only one node if it detects a stateless property is being tested, or a new node per worker in the case of a stateful property.
As `lists:zip/2` expects two lists of the same length, it would crash on stateful properties when `numtests` was less than `num_processes`. In that case, we only need to spawn `numtests` processes (since otherwise we would end up having idle workers).
Also did some refactor, mainly changin from *processes* to *workers* (and so `num_processes` to `num_workers`, etc), and added some documentation.
Also enforce a more strict pattern matching when trying to determine if a property is stateless or stateful.
Instead of adding a workflow to run the test suite with parallel PropEr, we should have a target in the Makefile to run the examples with it. So, this commit removes all references to the `num_workers` option in the suite except for the test case that runs the examples, `examples_are_ok_test_/0`, as we do want to test them in parallel and the test case will be removed in the future.
This commit can and will be dropped at a future point in time, and is only being added to help continue testing parallel PropEr in Github Actions.
src/proper.erl
Outdated
| -spec parallel_perform(test(), opts()) -> imm_result(). | ||
| parallel_perform(Test, #opts{property_type = pure, numtests = NumTests, | ||
| numworkers = NumWorkers, strategy_fun = StrategyFun} = Opts) -> | ||
| TestsPerWorker = StrategyFun(NumTests, NumWorkers), |
There was a problem hiding this comment.
I would suggest to factor out all lines between
TestsPerWorker = StrategyFun(NumTests, NumWorkers),
....
ok = maybe_stop_cover_server([]),into a separate function and use it both for this clause and the next one (by passing it [] or NodeList).
This will help both for maintainability and for understanding what is common and what is not.
There was a problem hiding this comment.
You also need to construct and pass to this function the appropriate SpawnFun, of course.
| perform(Passed, NumTests, Test, Opts) -> | ||
| Size = size_at_nth_test(Passed, Opts), | ||
| put('$size', Size), | ||
| perform(Passed, NumTests, 3 * ?MAX_TRIES_FACTOR * NumTests, Test, none, none, Opts). |
There was a problem hiding this comment.
You should probably add a comment where this magic number 3 comes from... what is its role.
src/proper.erl
Outdated
|
|
||
| %% @private | ||
| -spec update_worker_node_ref({node(), {already_running, boolean()}}) -> list(node()). | ||
| update_worker_node_ref(Node) -> |
There was a problem hiding this comment.
Node is a confusing variable name here...
src/proper.erl
Outdated
| %% @doc Starts multiple (NumNodes) remote nodes. | ||
| -spec start_nodes(non_neg_integer()) -> list(node()). | ||
| start_nodes(NumNodes) -> | ||
| StartNode = |
pablocostass
force-pushed
the
parallel_proper
branch
from
ae88c66
to
51b1f6d
Compare
|
@kostis As the commit that had added the As I am not really happy with how I did the refactoring of the common code that |
src/proper.erl
Outdated
| -spec parallel_perform(test(), opts()) -> imm_result(). | ||
| parallel_perform(Test, #opts{property_type = pure, numtests = NumTests, | ||
| numworkers = NumWorkers, strategy_fun = StrategyFun} = Opts) -> | ||
| _ = maybe_start_cover_server([]), |
There was a problem hiding this comment.
This line / call is not needed here; it will be done by spawn_workers_and_get_result. (Or am I missing something?)
There was a problem hiding this comment.
Oh no, that was totally my mistake, thanks for pointing it out!
There was a problem hiding this comment.
Amended the last commit to remove the line.
pablocostass
force-pushed
the
parallel_proper
branch
2 times, most recently
from
da99645
to
0cebf8a
Compare
pablocostass
force-pushed
the
parallel_proper
branch
from
0cebf8a
to
a2497b2
Compare
This PR extends PropEr with parallel execution. It is based on the work I did for my undergraduate thesis and has some limitations at the moment (e.g., targeted properties). Nonetheless, it should pretty much work (famous last words) and one can find the relevant options to test with parallel PropEr in the documentation of this branch, so please do take a look at it too.
The PR has two extra commits that I considered relevant but might be dropped upon request, those are:
Add verbosity to
make testoutput (df7a7d8)Adds verbosity when running the tests, as that way it is easier to check at what point parallel PropEr failed in the suite.
Add a workflow to test with parallel PropEr (4c81af9)
This commit adds a new workflow to be ran with Github Actions that uses 2 workers to run the test suite and ignores failures in the workflow; still, the warnings/failures are recorded by GA, so they will not be lost. This way, no PR would be blocked by some flakyness coming from parallel PropEr.
In short, this PR brings four new options to the table, all related to this new kind of execution:
{numworkers, <Non_negative_number>}sets the number of workers to use during testing.pure(side effect free) orimpure(with side effects) tells PropEr the kind of property it is going to test;impureproperties start nodes to isolate the workers from the others and avoid possible test clashes.{strategy_fun, <Strategy_function>}overrides the default function that divides the workload among the total of workers.{stop_nodes, true | false}tells PropEr whether it should stop the nodes (if any was started) after finishing testing. This one is mainly used forproper:module/1,2, but it will probably be useful for some (e.g., build tools).