Skip to content
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

Misc Cleanup/Minor Fixes from KVS TreeObject Work #1152

Merged
merged 8 commits into from Aug 17, 2017

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Aug 17, 2017

Spliced out set of cleanup patches or minor bug fixes from TreeObject work both @garlick and I have been working on.

Note that @garlick has one patch in this PR, but it's only addition of a copyright header. Dunno if its safe for him to hit the button. But copyright header probably counts as "non-source documentation" (https://github.com/flux-framework/rfc/blob/master/spec_1.adoc).

garlick and others added some commits Jul 27, 2017

modules/kvs: Fix invalid cleanup on error
json_array_get() returns a borrowed reference, not a copy, so on
error path the json object should not have its reference count
decreased.
modules/kvs/test: Add new commit test
In commit_basic_commit_process_test_multiple_fences(), make one
of the commits in a directory vs the root dir, to mix up the workload.
modules/kvs/test: Add missing tests
Add missing function call to run some commit unit tests.
modules/kvs: Remove unnecessary cleanup
Remove cleanup on error path that is unnecessary.  The item_callback_list
in this particular area of code will always be empty if it has reached
this particular error path.
t/kvs: Fix corner case in watch tests
Fix bug which error in watch callbacks was not detected and tests
could still pass.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 17, 2017

Coverage Status

Changes Unknown when pulling 331763d on chu11:misccleanup7 into ** on flux-framework:master**.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Aug 17, 2017

I restarted the clang build that failed with the "write error" problem (#1145)

rm -f TAGS ID GTAGS GRTAGS GSYMS GPATH tags
test . = "../../../../../src/common/libkz" || test -z "" || rm -f 
rm -rf ./.deps
rm -f Makefile
make[4]: Leaving directory `/home/travis/build/flux-framework/flux-core/flux-core-d6a4311/_build/sub/src/common/libkz'
make[4]: write error
make[3]: *** [distclean-recursive] Error 1
make[3]: Leaving directory `/home/travis/build/flux-framework/flux-core/flux-core-d6a4311/_build/sub/src/common'
make[2]: *** [distclean-recursive] Error 1
make[2]: Leaving directory `/home/travis/build/flux-framework/flux-core/flux-core-d6a4311/_build/sub/src'
make[1]: *** [distclean-recursive] Error 1
make[1]: Leaving directory `/home/travis/build/flux-framework/flux-core/flux-core-d6a4311/_build/sub'
make: *** [distcheck] Error 1
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Aug 17, 2017

Looks good to me.

Is 21947fc one that could go in this PR?

commit  21947fc290c53058ddd0faaf610caa1dbf67f609
Author: Albert Chu <chu11@llnl.gov>
Date:   Mon Aug 14 16:10:45 2017 -0700

    t/kvs: Fix error in null tests
    
    In original test of setting the string "null" within the KVS there
    was an error that worked out by chance but was incorrect.
    
    When doing a kvs put with the string "null", such as with:
    
    flux kvs put key=null
    
    this should create a json null object.  On the other hand:
    
    flux kvs put key=\"null\"
    
    should create a string "null".
    
    Correct for this and create tests for both scenarios.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 17, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@8e439c6). Click here to learn what that means.
The diff coverage is 0%.

@@           Coverage Diff            @@
##             master   #1152   +/-   ##
========================================
  Coverage          ?   77.5%           
========================================
  Files             ?     157           
  Lines             ?   28755           
  Branches          ?       0           
========================================
  Hits              ?   22286           
  Misses            ?    6469           
  Partials          ?       0
Impacted Files Coverage Δ
src/common/libkvs/kvs_txn.c 61.14% <ø> (ø)
src/modules/kvs/fence.c 82.5% <ø> (ø)
src/modules/kvs/commit.c 76.77% <ø> (ø)
src/bindings/lua/flux-lua.c 81.67% <0%> (ø)
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 17, 2017

fyi - codecov is complaining because the last coverage build on master failed. I kicked it, so that should be resolved in an hour or so.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Aug 17, 2017

@garlick I skipped that one b/c the old code in master actually behaves incorrectly. So travis would fail. It'll all be corrected in the treeobj PR.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Aug 17, 2017

OK, sounds good. I'll just go ahead and merge this without the updated coverage results since this touches very little code.

@garlick garlick merged commit 0337f01 into flux-framework:master Aug 17, 2017

2 of 4 checks passed

codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 77.867%
Details

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.