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

Cleanup KVS tests #1149

Merged
merged 8 commits into from Aug 14, 2017

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Aug 11, 2017

This PR cleans up a lot of the KVS tests, as legacy testing lead to duplicate code, duplicate tests, and unnecessary stuff. As well as fixing up #1127, this should make it easier to convert the KVS tests
over to treeobj at a later date.

In the middle of all this, I added the -f option to flux kvs unlink, as there are several circumstances where we want to delete stuff before a test, but don't want flux kvs unlink to return an error if that key/directory is missing.

Note that I'd like to rename the files t1000-kvs-basic.t and t1002-kvs-cmd.t, as the tests are no longer "basic" and the "cmd" tests are no longer just about the kvs command. I'll rename appropriately at a later time (probably at a patch to end of this PR), as it may be hard to review this PR if I rename the files and all my changes appears to be new files.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 11, 2017

Coverage Status

Coverage decreased (-0.003%) to 77.967% when pulling 426ef92 on chu11:kvsbasic-4 into 1665912 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Aug 11, 2017

Huh, I got 3 build failures in travis. One hit #997, so I restarted and that passed. But in the other ones, all unit tests passed. So I'm not entirely sure why it failed. Travis didn't appear to time out either.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 11, 2017

Search for write error in the output? Could be #1145.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Aug 11, 2017

@grondo Thanks, seems to be it. /bin/bash: line 5: echo: write error: Resource temporarily unavailable

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 11, 2017

Codecov Report

Merging #1149 into master will increase coverage by 0.04%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #1149      +/-   ##
==========================================
+ Coverage    77.6%   77.65%   +0.04%     
==========================================
  Files         157      157              
  Lines       28693    28698       +5     
==========================================
+ Hits        22267    22285      +18     
+ Misses       6426     6413      -13
Impacted Files Coverage Δ
src/cmd/flux-kvs.c 76.62% <88.88%> (+1.62%) ⬆️
src/common/libflux/info.c 75% <0%> (-2.78%) ⬇️
src/common/libcompat/info.c 65.82% <0%> (-2.54%) ⬇️
src/broker/modservice.c 79.61% <0%> (-1.95%) ⬇️
src/broker/overlay.c 71.67% <0%> (-0.35%) ⬇️
src/broker/module.c 83.28% <0%> (-0.28%) ⬇️
src/bindings/lua/flux-lua.c 81.67% <0%> (-0.1%) ⬇️
src/common/libflux/message.c 81.29% <0%> (+0.11%) ⬆️
src/common/libcompat/handle.c 84.94% <0%> (+0.23%) ⬆️
src/common/libflux/handle.c 83.66% <0%> (+0.24%) ⬆️
... and 6 more
flux kvs link $TEST.a.b.X $TEST.a.b.link &&
ROOTREF=$(${KVSBASIC} get-treeobj .) &&
${KVSBASIC} unlink $TEST &&
flux kvs unlink -R $TEST &&

This comment has been minimized.

@garlick

garlick Aug 13, 2017

Member

did a -f get missed here?

This comment has been minimized.

@chu11

chu11 Aug 14, 2017

Author Contributor

I think we keep this one at -R b/c we want the unlink to pass given the path created above it.

@@ -665,7 +665,7 @@ test_expect_success 'kvs: async kvs_fence allows puts with fence in progress' '

# base64 data
test_expect_success 'kvs: copy-tokvs and copy-fromkvs work' '
${KVSBASIC} unlink $TEST &&
flux kvs unlink -R $TEST &&

This comment has been minimized.

@garlick

garlick Aug 13, 2017

Member

missed -f here too?

This comment has been minimized.

@chu11

chu11 Aug 14, 2017

Author Contributor

yup thanks

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Aug 13, 2017

Looks good - I probably should have removed the duplicate code/tests when I made the last round of KVS API changes. Thanks for doing that. I'm OK with renaming the tests too.

chu11 added some commits Aug 10, 2017

t/kvs: Remove duplicate kvs test code
Remove get, put, version, wait, dropcache, copy, move, mkdir, link,
readlink in t/kvs/basic, as it is basically a duplicate of what is
in the flux-kvs command.

Update t1000-kvs-basic.t tests appropriately, using 'flux kvs' equivalent
as needed.

Fixes #1127
cmd/kvs: Add -f option to unlink
Add -f (force) option to unlink command to allow unlink to return
a successful error code even if a file is not found.  Maps loosely
to the -f option in rm.
t/kvs: Update t1002-kvs-cmd.t to use unlink -f
In locations where we are "cleaning up" before a test, use unlink -f,
because we don't care if a prior test failed and the unlink isn't
removing anything.
t/kvs: Remove duplicate kvs test code
Remove unlink in t/kvs/basic, as it is basically a duplicate of what is
in the flux-kvs command.

Update t1000-kvs-basic.t tests appropriately, using 'flux kvs unlink'
as needed.
t/kvs: Remove duplicate tests in t1000-kvs-basic.t
Remove tests that are duplicates of ones in t1002-kvs-cmd.t.
t/kvs: Move tests from t1000 to t1002
Move tests that only use flux kvs command from t1000-kvs-basic.t to
t1002-kvs-cmd.t.
t/kvs: Remove unnecessary test code
Remove kvs/basic exist command, as it is basically the same
as flux kvs get, but just not outputting the result.

Adjust tests appropriately for change.
t/kvs: Rename kvs test files
Rename t1002-kvs-cmd.t to t1000-kvs.t, as it is now the primary kvs
test file.

Rename t1000-kvs-basic.t to t1002-kvs-extra.t, as it now provides
the extra kvs tests we want to coverage and stress testing.

@chu11 chu11 force-pushed the chu11:kvsbasic-4 branch from 426ef92 to 72e8a14 Aug 14, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Aug 14, 2017

rebased on master and fixed the unlink -R @garlick mentions above.

renamed t1002-kvs-cmd.t to `t1000-kvs.tas it is now the main kvs test. t1000-kvs-basic.t``` is now ```t1002-kvs-extra.t```, as it handles stress testing and other testing.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 14, 2017

Coverage Status

Coverage increased (+0.05%) to 77.997% when pulling 72e8a14 on chu11:kvsbasic-4 into aeb6cfd on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 14, 2017

restarted one of the builds which hit the write error issue

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 14, 2017

Looks good and since @garlick already reviewed this I'll merge it unless I hear otherwise

@grondo grondo merged commit fc2a4ff into flux-framework:master Aug 14, 2017

4 checks passed

codecov/patch 88.88% of diff hit (target 77.6%)
Details
codecov/project 77.65% (+0.04%) compared to aeb6cfd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 77.997%
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.