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: allow get_double to be called on an int #872

Merged
merged 1 commit into from Oct 25, 2016

Conversation

Projects
None yet
4 participants
@SteVwonder
Copy link
Member

SteVwonder commented Oct 25, 2016

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Oct 25, 2016

Doh, I got so caught up on fixing the simulator to verify this patch worked that I forgot to create tap tests. Gimme some more time to push those before merging. Sorry

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Oct 25, 2016

@grondo or @garlick what do you think is the best way to test this?

As far as I can tell, the tests in /src/modules/kvs/ aren't run within a Flux instance. Is it possible to run these unit tests within an instance?

I know that the tests in /t/ are run under a flux instance, but the kvs tests there utilize the flux-kvs command. The flux-kvs command does just a straight kvs_put so the values aren't put through type-specific json serialization. I could extend the flux-kvs command to support something like flux kvs put-as double "foo" 1.0.

Thought?

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 25, 2016

I think the current flux kvs put and flux kvs type will let you put a double or an int (e.g. 1.0 vs 1) and test what json-c thinks it is. Maybe what's lacking is flux kvs get-as?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 25, 2016

Coverage Status

Coverage decreased (-0.03%) to 75.721% when pulling 86218df on SteVwonder:kvs-type-fix into 14b9ef1 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 25, 2016

I think the current flux kvs put and flux kvs type will let you put a double or an int (e.g. 1.0 vs 1) and test what json-c thinks it is. Maybe what's lacking is flux kvs get-as?

One thing I was seeing is that I could flux kvs put foo=1.0 then flux kvs get-treeobj showed the value stored as "1.0". I wonder how that is happening:

$ flux kvs put foo=1.0
$ flux kvs get-treeobj foo
{"FILEVAL":1.0}

I think flux kvs put just uses kvs_put (h, key, json_str) where above json_str is "1.0". In this case the JSON is parsed into an object and I'm guessing json-c preserves the string representation and doesn't regenerate it.

For the tests we should use kvs_put_double() directly. Perhaps using a little test program under ./t?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 25, 2016

Oh @garlick, I apologize as I think I misread what you were suggesting for the test. The test could put an integer and double and make sure both can be read back as double using kvs get-as. Sorry I think that would be sufficient here.

What if instead of adding a new one-off command to flux kvs we somehow allowed jansson style unpack format specifier to be used with both get and put. I'm not sure how the get would actually work, but just an idea to throw out there.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 25, 2016

@SteVwonder, I don't think we necessarily need to wait for a test on this one, since the test will end up being non-trivial. I'll open an issue to track that we should check both integer and float values can be read with kvs_get_double(), and once travis returns (had to restart one test) I think we can merge this since it fixes a real regression/bug.

@grondo grondo merged commit b7c84cf into flux-framework:master Oct 25, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 75.721%
Details

@SteVwonder SteVwonder deleted the SteVwonder:kvs-type-fix branch Feb 16, 2019

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.