Test cleanup #240

tburghart · 2017-01-28T20:45:17Z

This started with fixing tests to NOT leave test files all over the place, but it quickly became apparent that many of the tests weren't running at all.

Turn off whitespace diffs by appending ?w=1 (or &w=1, as appropriate) to the URL.

With these changes:

All tests actually run, and pass.
A few tests have been added that seemed appropriate.
All tests write only in /tmp and clean up after themselves.
All relevant cases of failures that would throw a badmatch are now enclosed in appropriate ?assert... macros.
The fundamental behavior of the tests hasn't been changed.

Note: This does nothing about the cuttlefish kludge, sorry MvM.

This PR replaces #239 with a branch name that should be compatible with all builders.

A commit over 4 years ago disabled most of the eunit tests in the eleveldb module. This enables them and adds a few, while refactoring so they clean up after themselves. It also adds exported test helper functions to make it easier for other tests to clean up after themselves.

Tests now use helper functions from eleveldb module tests.

…time out on wimpy builders.

- Number of database instances is capped to keep filesystem footprint manageable. - Complexity of some operations reduced to limit runtime. - Generous timeouts added to more tests.

thumbot · 2017-01-29T13:00:22Z


trb-test-cleanup `db0d66d`	➡️	develop `996cb43`	✅ completed

Looks good! 👍

✅ MERGE

Started at: 2017-01-29 05:00
Duration: 0 seconds.
Result: OK
Message: Merge Success: trb-test-cleanup db0d66d onto target branch: develop 996cb43
Exit Code: OK

📄







  Updating 996cb43..db0d66d
Fast-forward (no commit created; -m option ignored)
 src/eleveldb.erl   | 498 ++++++++++++++++++++++++++++++++++++++---------------
 test/cacheleak.erl |  82 +++++----
 test/cleanup.erl   | 224 ++++++++++++------------
 test/iterators.erl | 181 +++++++++----------
 4 files changed, 606 insertions(+), 379 deletions(-)

matthewvon · 2017-01-29T17:27:58Z

I have executed the buildbot automation against this branch. There were 4 failing platforms out of 16. This is the typical count. I manually reviewed the failing four. Two executed fine upon manual retest. The other two (smartos1.8 and freebsd10) were typical resource problems. Not going to sweat it. I +1 from the automation viewpoint.

JeetKunDoug

Just some stylistic/consistency notes on formatting - otherwise looks good.

JeetKunDoug · 2017-02-01T21:42:38Z

src/eleveldb.erl

+%% Exported Test Helpers
+%% ===================================================================
+
+-spec assert_close(DbRef :: db_ref()) -> ok | no_return().


The "spec, comments, function" pattern you're using for some reason really messes with me - we almost always have doc comments above spec and function declarations. Can you please swap these around and, where they are public functions (maybe not too many) make them @doc comments?

Only other thing (and again, general style note) is that left-aligned comments generally have 2 % characters, so all the comments above functions should be double-% - some sort of emacs-mode standard, and things can get messed up if you aren't consistent about it.

All 42 tests pass locally... if you could do the minor cleanup stuff I'll +1.

I use a single % prefix when it's not published in edoc.
If it doesn't have a %% @doc comment, it's because it's not exported in all cases, and edoc will blow up on it. Basically, anything within the -ifdef(TEST). block with any edoc comments causes failure (edoc's not very smart sometimes).

The "spec, comments, function" pattern is not at all uncommon in Erlang code, and I use it because I find it easier to digest when the spec doesn't run immediately into the code

Problem is it's not common in Basho code - we pretty much universally do it the other way (comments then spec then function) - consistency with the rest of the codebase is important. Same goes for 2 % symbols for left-aligned comments (even when not @doc comments) - I've almost never seen single-% comments in our codebase, which is why it stuck out at me like a sore thumb.

I refer you to the edoc documentation:

Examples:

-spec my_function(X :: integer()) -> integer(). %% @doc Creates ...

The first example shows the recommended way of specifying functions.

There may be more of it in our source base than you think - it's not all from me.

The latest commit takes care of the comment format.

Please go take a look at almost any other use of a -spec and a comment above a function in our codebase (specifically, modules starting with riak_ since we "own" those more than others)... In every case I've looked at (and I'm at about 100 instances of -spec so far) it's doc/spec/function. I think keeping consistent with the rest of our codebase is important enough to spend some time swapping these around.

…Doug and/or emacs happy (the former is the more worthy goal). Sync with latest 'develop' branch.

thumbot · 2017-02-04T12:42:57Z


trb-test-cleanup `89a5756`	➡️	develop `254cba3`	✅ completed

Looks good! 👍

✅ MERGE

Started at: 2017-02-04 04:42
Duration: 1 seconds.
Result: OK
Message: Merge Success: trb-test-cleanup 89a5756 onto target branch: develop 254cba3
Exit Code: OK

📄







  Updating 254cba3..89a5756
Fast-forward (no commit created; -m option ignored)
 src/eleveldb.erl   | 502 ++++++++++++++++++++++++++++++++++++++---------------
 test/cacheleak.erl |  82 +++++----
 test/cleanup.erl   | 224 ++++++++++++------------
 test/iterators.erl | 181 +++++++++----------
 4 files changed, 610 insertions(+), 379 deletions(-)

JeetKunDoug · 2017-02-13T20:17:38Z

+1

tburghart added 4 commits January 28, 2017 09:29

Fix tests so they clean up after themselves.

8780b3d

Tests now use helper functions from eleveldb module tests.

Make test_open_many relative to the number of schedulers so it won't …

31952a9

…time out on wimpy builders.

More tweaks to handle lightweight builder VMs.

41f67a0

tburghart mentioned this pull request Jan 28, 2017

Test cleanup #239

Closed

Another try at not overloading test platforms.

db0d66d

- Number of database instances is capped to keep filesystem footprint manageable. - Complexity of some operations reduced to limit runtime. - Generous timeouts added to more tests.

JeetKunDoug reviewed Feb 1, 2017

View reviewed changes

Change left-margin single-percent comments to double-percent to keep …

89a5756

…Doug and/or emacs happy (the former is the more worthy goal). Sync with latest 'develop' branch.

tburghart merged commit 4e63b92 into develop Feb 13, 2017

tburghart deleted the trb-test-cleanup branch February 13, 2017 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test cleanup #240

Test cleanup #240

tburghart commented Jan 28, 2017 •

edited

Loading

thumbot commented Jan 29, 2017 •

edited

Loading

matthewvon commented Jan 29, 2017

JeetKunDoug left a comment

JeetKunDoug Feb 1, 2017

JeetKunDoug Feb 1, 2017

JeetKunDoug Feb 1, 2017

tburghart Feb 3, 2017

tburghart Feb 3, 2017

JeetKunDoug Feb 3, 2017

tburghart Feb 4, 2017 •

edited

Loading

tburghart Feb 4, 2017

JeetKunDoug Feb 8, 2017

thumbot commented Feb 4, 2017 •

edited

Loading

JeetKunDoug commented Feb 13, 2017

Test cleanup #240

Test cleanup #240

Conversation

tburghart commented Jan 28, 2017 • edited Loading

thumbot commented Jan 29, 2017 • edited Loading

matthewvon commented Jan 29, 2017

JeetKunDoug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tburghart Feb 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thumbot commented Feb 4, 2017 • edited Loading

JeetKunDoug commented Feb 13, 2017

tburghart commented Jan 28, 2017 •

edited

Loading

thumbot commented Jan 29, 2017 •

edited

Loading

tburghart Feb 4, 2017 •

edited

Loading

thumbot commented Feb 4, 2017 •

edited

Loading