Enchance/mw/bg manager #538
Conversation
The table is not given to the bg_manager. The supervising process creates the tables named and public. The bg_manager always refers to the tables by name, so there's waiting for a transfer.
The more similar the code used in test is to production, the better. If the test needs to survive killing the manager, unlink can be used.
Most tests are there to ensure the external api remains consistent and cover the interesting cases. This has boosted coverage from 60's to 90's. This also caught incorrect specs for some return values.
|
Is this targeted for 2.0 or 2.1? |
| gen_server:call(?SERVER, {disable, Resource}). | ||
| case gen_server:call(?SERVER, {disable, Resource}) of | ||
| {unregistered, Resource} -> | ||
| unregistered; |
There was a problem hiding this comment.
what's the benefit of changing the API here. I understand the information is redundant but is there a reason? Also, this code path seems repeated in many places.
There was a problem hiding this comment.
The spec did not match the actual return value. Looking at where this was used, it seemed the better option was to change the code so it matched the spec rather than the other way around. This was discovered while writing the unit test for the function.
There was a problem hiding this comment.
hrm, neither dialyzer [1] nor the eqc test caught this. one should...once again, i'd like to move away from the unit tests (and see them removed once any non-redundant parts are part of the eqc test). the unit tests caused several problems along the way during review.
[1] https://github.com/basho/riak_core/pull/528/files#diff-9
There was a problem hiding this comment.
Double checking, this breaks the eqc, so fixing the spec would been the right way to go.
There was a problem hiding this comment.
ok wfm. i'm guessing btw that dialyzer can't figure it out bc of the gen_server message boundary.
|
This has been punted upon until at least 2.1. |
|
related PR: #535 |
|
For now marking as milestone @lordnull can this be closed? some of these changes overlapped with other PRs and others, and iirc broke tests or were not necessary. |
|
This can be closed mainly since it ended up duplicating some effort. |
Add tests for coverage of exported api, and removed unused/old code in the process.