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

kvs test cleanup #1313

Merged
merged 8 commits into from Jan 4, 2018

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Jan 3, 2018

Some simple kvs test cleanup I wanted to do. Mostly put some common functions into kvs/kvs-helper.sh, but some tiny things beyond that too.

chu11 added some commits Jan 3, 2018

t/t1002-kvs-watch.t: Minor function refactor
Rename wait_watch_current() to wait_watch_file() and take filename
as argument.
t/t1002-kvs-watch.t: Minor code cleanup
Make loop iteration configurable via new KVS_WAIT_ITERS variable.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 3, 2018

Coverage Status

Coverage decreased (-0.5%) to 78.088% when pulling 3116a81 on chu11:kvscleanup8 into d29b0cd on flux-framework:master.

@chu11 chu11 force-pushed the chu11:kvscleanup8 branch from 3116a81 to b380a48 Jan 3, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 3, 2018

Codecov Report

Merging #1313 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1313      +/-   ##
==========================================
- Coverage   78.24%   78.23%   -0.02%     
==========================================
  Files         154      154              
  Lines       28100    28100              
==========================================
- Hits        21987    21984       -3     
- Misses       6113     6116       +3
Impacted Files Coverage Δ
src/common/libkvs/kvs_watch.c 87.66% <0%> (-0.89%) ⬇️
src/common/libutil/base64.c 95.07% <0%> (-0.71%) ⬇️
src/broker/overlay.c 73.88% <0%> (-0.64%) ⬇️
src/modules/connector-local/local.c 74.38% <0%> (-0.21%) ⬇️
src/common/libflux/message.c 81.6% <0%> (+0.11%) ⬆️
src/common/libflux/handle.c 83.91% <0%> (+0.24%) ⬆️
src/common/libflux/request.c 89.74% <0%> (+1.28%) ⬆️
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 3, 2018

oops, bourne shell uses . and not source. Re-pushed with fixes.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 4, 2018

Coverage Status

Coverage decreased (-0.01%) to 78.562% when pulling b380a48 on chu11:kvscleanup8 into d29b0cd on flux-framework:master.

chu11 added some commits Jan 3, 2018

t/t1002-kvs-watch.t: Move helper functions
Move helper functions into new kvs/kvs-helper.sh file.
t/kvs/kvs-helper.sh: Add loophandlereturn()
Use in t1004-kvs-namespace.t to cleanup lines of code.
t/t1004-kvs-namespace.t: Rename function
For consistency, rename wait_watch_namespace_put().

@chu11 chu11 force-pushed the chu11:kvscleanup8 branch from b380a48 to 9475591 Jan 4, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 4, 2018

oops, forgot to add helper file to dist. Re-pushed again.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 4, 2018

Hit #731, restarted a builder

FAIL: t0016-cron-faketime.t 3 - flux-cron tab works for set minute

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 4, 2018

Coverage Status

Coverage decreased (-0.01%) to 78.558% when pulling 9475591 on chu11:kvscleanup8 into d29b0cd on flux-framework:master.

@chu11 chu11 force-pushed the chu11:kvscleanup8 branch from 9475591 to cf3c416 Jan 4, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 4, 2018

noticed a minor issue with a comment, repushed

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 4, 2018

Coverage Status

Coverage remained the same at 78.572% when pulling cf3c416 on chu11:kvscleanup8 into d29b0cd on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jan 4, 2018

This looks good to me. If you're done tweaking it, I vote we merge.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jan 4, 2018

I'm not commenting on this PR specifically, however if someone does more cleanup in the future, you could see if the busywaiting in wait_watch_file() could be replaced with t/scripts/waitfile.lua which uses libev statwatcher to more efficiently wait for a line in a file to match a pattern.

(I didn't look closely at what wait_watch_file actually does so I apologize in advance if the waitfile helper script doesn't apply here)

chu11 added some commits Jan 3, 2018

t/t1004-kvs-namespace.t: Minor cleanup
Use KVS_WAIT_ITERS instead of hard coding 50.

Call wait_watch_put() to remove duplicate code.

General code cleanup.
t/kvs: Add test_kvs_key to kvs helper
Adjust t1000-kvs and t1004-kvs-namespace accordingly.

@chu11 chu11 force-pushed the chu11:kvscleanup8 branch from cf3c416 to 15b4363 Jan 4, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 4, 2018

@garlick i thought of one extra cleanup this morning, so I just pushed that. Otherwise, it's good to go.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 4, 2018

@grondo I wasn't aware of the waitfile.lua script, but it seems (glossing over the help comments) like it could probably work. Will have to investigate in the future.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 4, 2018

Coverage Status

Coverage decreased (-0.01%) to 78.562% when pulling 15b4363 on chu11:kvscleanup8 into d29b0cd on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jan 4, 2018

Hmm, any idea why that last change dropped the project coverage into the red? Did something stop running I wonder?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jan 4, 2018

Hmm, any idea why that last change dropped the project coverage into the red? Did something stop running I wonder?

The coverage only changed -0.02% which is negligible (and probably caused by coverage "noise" that I don't quite understand yet). Codecov should have some tunable that indicates only a drop in coverage of X% should turn the report red (I thought we'd set that already)

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jan 4, 2018

Oh right, I guess that is not anything to worry about. I'll go ahead and merge.

@garlick garlick merged commit 6cd7cf7 into flux-framework:master Jan 4, 2018

4 of 5 checks passed

codecov/project 78.23% (-0.02%) compared to d29b0cd
Details
buildbot/core_standard Build done.
Details
codecov/patch Coverage not affected when comparing d29b0cd...15b4363
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 78.562%
Details

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

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.