-
Notifications
You must be signed in to change notification settings - Fork 232
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
Fix eunit failures #360
Fix eunit failures #360
Conversation
I haven't reviewed the code but it did fix eunit failures. |
|
||
|
||
do_dep_apps(StartStop, Apps) -> | ||
lists:map(fun(A) when is_atom(A) -> application:StartStop(A); |
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.
application:unload/1
has been known to do strange things in the past. But, I think that may have been before we started running each app's eunits as a separate rebar invocation. If it passes all platforms now, we can return to its debugging when/if necessary.
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 think my goal was to remove all application env entries for the app, and then load them fresh on the next run from the .app file. If unload doesn't do that, then it can easily be removed.
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, I must admit to cargo-culting (i.e. I'm not sure what else it does). @slfritchie has warned me off when I've snuck in calls to unload before.
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.
application:unload() can occasionally fail with an exception that cascades to the (IIRC) the application controller and then crashes the OTP kernel app, which then stops the VM. Tests interpret the VM shutting down as a failure, which I think is a good feature. I forget the details of how it works, but it does happen, at least with R14B04. I haven't tried risking the ire of R15B01.
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.
@slfritchie Is it only certain apps? I could understand why unloading kernel or stdlib would do that.
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 couldn't find a pattern, but it wasn't a matter of unloading apps with sticky dirs like kernel. It can happen for any of the Riak dependency apps.
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.
Either way, i removed the unloading bit. Not having it doesn't seem to affect any of our existing tests.
Running locally (OS X 10.8, Erlang R15B01), |
* New keys_fsm requires ack_keys when backpressure is turned on. * Correct the startup order of applications based on reltool's computed list. * Use the memory backend and its reset() function instead of deleting keys. * Isolate the state directory of the test to .eunit/keys_fsm_eqc. * Cleanup ring files in the state directory before the test instead of rejiggering the ring. * Use a consistent node name and use longnames so that the put_fsm doesn't get confused. * Wait for KV application and service to start rather than using an arbitrary sleep.
…e can use it elsewhere.
… eaddrinuse errors.
* Don't call application:unload/1. * Don't start folsom since it is an included application of riak_core. * Improve the documentation on new riak_kv_test_util functions.
%% `CleanupFun' given to `cleanup/3'. | ||
%% | ||
%% @see common_setup/2, dep_apps/2, do_dep_apps/2 | ||
-spec setup(TestName::string(), fun((load|start|stop) -> any()) -> ok. |
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.
There is no need to put the TestName
in the spec. Since sometime in release 14 edoc is smart enough to match specs and function arguments up automatically.
Why not put Also, that module failed to compile for me:
|
The tests pass and based on the output seem to run more cleanly as well. Nice job @seancribbs. +1 to merge. |
This fixes a bunch of failures and setup problems in
riak_kv
's eunit/EQC suite. It is currently passing on Travis.