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: cleanup / re-organize sharness tests #1276

Merged
merged 20 commits into from Nov 8, 2017

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Nov 7, 2017

Due to various changes in the KVS and flux-kvs the past few months, the kvs sharness tests have gotten a little disorganized, so I thought it'd be a good idea to go through them before we move onto the next big work.

A number of tests were eliminated because they were deemed unnecessary, most notably legacy tests related to all KVS objects being stored as json (everything is stored raw text now) and tests for now-gone functions.

There were a decent chunk of duplicate (or near duplicate) tests lingering, got rid of the duplicates.

Re-organized the KVS tests into four files

  • t1000-kvs - basic/core tests

  • t1001-kvs-internals - "non obvious" tests that are really to test KVS server internals (e.g. kvs server turns large values into valrefs). This is for stuff that would generally be regarded as non-obvious to users.

  • t1002-kvs-watch - kvs watch tests. These are sort of a unique beast by themselves, so split them off.

  • t1003-kvs-stress - running programs written to stress test the KVS

To accomodate the naming of these files, some other test files were re-numbered.

Admit that tests could be split up other ways. I went back and forth on a few different ideas, but this is what I settled on. Minimally its far more organized compared to what was there before.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 7, 2017

Maybe update the (grossly outdated) t/README.md to indicate that t10[00-99]*.t are reserved for kvs tests? not sure what other ranges we've reserved by accident...

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 7, 2017

Coverage Status

Coverage decreased (-0.004%) to 78.504% when pulling e0ee660 on chu11:issue1259 into 4456339 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 7, 2017

oh, wasn't even aware of the t/README.md, will adjust.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 7, 2017

Ah really nice to have this split up and organized!

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 7, 2017

It looks like a nice break down. My only comment would be that it might be good to move the five watch tests that are in the stress script over to the watch script. They get done pretty fast.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 7, 2017

oh boy the README is way out of date

 - 0 - basic tests. Verify testsuite and basic command functionality.
 - 1 - kvs tests
 - 2 - TBD
 - 3 - ...

I'm not entirely sure what pattern the tests may be following at this point. Especially when including the ones in t/lua. I'm thinking maybe we should open another issue, because the naming probably has to be cleaned up across many areas.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 7, 2017

I'm thinking maybe we should open another issue, because the naming probably has to be cleaned up across many areas

Sounds good to me.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 8, 2017

huh ... 3 builds failed with

FAIL: t0001-basic.t 46 - flux-help command can display manpages for api calls

I'm a bit at a loss what I could have done to cause this. Double checked and didn't accidentally introduce some random typo into that file.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 8, 2017

Codecov Report

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

@@            Coverage Diff            @@
##           master    #1276     +/-   ##
=========================================
- Coverage   78.02%   77.92%   -0.1%     
=========================================
  Files         154      154             
  Lines       29097    29097             
=========================================
- Hits        22702    22674     -28     
- Misses       6395     6423     +28
Impacted Files Coverage Δ
src/common/libflux/ev_flux.c 97.56% <0%> (-2.44%) ⬇️
src/modules/connector-local/local.c 76.07% <0%> (-1.5%) ⬇️
src/broker/modservice.c 80.58% <0%> (-0.98%) ⬇️
src/common/libkvs/kvs_watch.c 87.55% <0%> (-0.89%) ⬇️
src/common/libflux/msg_handler.c 86.09% <0%> (-0.76%) ⬇️
src/connectors/local/local.c 87.4% <0%> (-0.75%) ⬇️
src/common/libflux/message.c 81.25% <0%> (-0.59%) ⬇️
src/modules/kvs/kvs.c 63.49% <0%> (-0.52%) ⬇️
src/common/libkvs/kvs.c 64.9% <0%> (-0.5%) ⬇️
src/common/libflux/reactor.c 93.44% <0%> (-0.29%) ⬇️
... and 3 more
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 8, 2017

My only comment would be that it might be good to move the five watch tests that are in the stress script over to the watch script.

I went back and forth on this. Could go either way. I'll move em in there.

chu11 added some commits Nov 6, 2017

t/kvs: Fix typos/comments
Remove various typos and legacy comments.
t/kvs: Remove json type tests
Remove tests related to the json type stored in the KVS.  The KVS
now stores data in raw format, and many of the original kvs_get_type (i.e.
kvs_get_double, kvs_get_int64) are now gone.  So the tests no longer
seem necessary.  The user is simply responsible for storing the data
in the KVS they want.

As a consequence, the test binaries kvs/basic and kvs/getas are no longer
used, so remove those files.
t/t1002-kvs-extra: Remove duplicate tests
As a result of prior removal of json type tests, remove tests
that are now duplicates of ones in t1000-kvs.t.  Remove helper
functions appropriately.
t/t1000-kvs: Re-org empty string tests
Put empty string tests into new section of tests.  Add
additional empty string tests.
t/t1000-kvs: Re-org json corner case tests
Re-organize several json corner case tests into a new section of
tests.
t/t1000-kvs: Reorganize basic tests
Reorganize most of the "basic" tests in t1000-kvs.t.  Instead of
having many tests mixed together and dependent on previous
tests executing properly, group into more sections testing
groups of functionality.  Make groups of tests largely independent
of prior group of tests.

Along the way, add a few more tests.
t/t1000-kvs: Move append tests
Move append tests with zero length value into zero length value
test group.
t/t1000-kvs: Re-organize ordering of tests
Reorganize tests.  Group corner case tests together, move them
closer/near the non-corner case equivalent tests.  Organize all
get/put option tests close to each other.  Organize all command
tests near each other.
t/kvs: Move kvs put --no-merge tests
Move kvs put --no-merge tests from t1002-kvs-extra to t1000-kvs.
t/kvs: Move kvs --treeobj tests
Move flux kvs --treeobj tests that were in t1002-kvs-extra into
t1000-kvs.  Remove tests that were duplicates.
t/t1002-kvs-extra: Remove dirsize test
Remove test that is left over legacy test from a now non-existent function.
t/t1000-kvs: Cleanup link/readlink tests
Reorganize link/readlink tests, splitting off corner case tests, remove
duplicate test, fix test descriptions.
t/kvs: Move kvs --at tests
Move flux kvs --at tests that were in t1002-kvs-extra into
t1000-kvs.
t/kvs: Move kvs non-stress tests
Move tests that are not stress tests from t1002-kvs-extra to
t1000-kvs.
t/t1002-kvs: Fix scripting style
Fix scripting style to be like t1000-kvs, most notably tests work
off $DIR instead of $TEST.
t/: Re-organize test files
To prepare for splitting up/renaming KVS tests, rename a number
of the other test files.

t1001-barrier-basic.t -> t1101-barrier-basic.t
t1005-cmddriver.t -> t1102-cmddriver.t
t1006-apidisconnect.t -> t1103-apidisconnect.t
t1007-kz.t -> t1104-kz.t
t1008-proxy.t -> t1105-proxy.t
t/t1002-kvs-watch: New test file
Splice out watch tests from t1000-kvs and t1003-kvs-stress.
t/t1001-kvs-internals: New test file
Splice out kvs "internals" tests from t1000-kvs.  These test internals
of the KVS that are generally non-obvious to users.

@chu11 chu11 force-pushed the chu11:issue1259 branch from 99bb2ef to 7e34bd7 Nov 8, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 8, 2017

had to rebase on master, so went ahead and squashed my dinky chain-lint fix and the move of the watch tests from t1003-kvs-stress to t1002-kvs-watch.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 8, 2017

Coverage Status

Coverage decreased (-0.08%) to 78.523% when pulling 7e34bd7 on chu11:issue1259 into 14943b8 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 8, 2017

Whee! Awesome.

@garlick garlick merged commit b10c1c7 into flux-framework:master Nov 8, 2017

3 of 4 checks passed

codecov/project 77.92% (-0.1%) compared to 14943b8
Details
codecov/patch Coverage not affected when comparing 14943b8...7e34bd7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.08%) to 78.523%
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.