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

JNA Binding: Fix of test_keySetCut_shouldPass #2557

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mirunix
Copy link

commented Mar 28, 2019

Basics

Check relevant points but please do not remove entries.
Do not describe the purpose of this PR in the PR description but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins and doc/METADATA.md)

Review

Remove the line below and add the "work in progress" label if you do
not want the PR to be reviewed:

@markus2330 please review my pull request and close #2551

Merging

Please add the "ready to merge" label when the build server and you say
that everything is ready to be merged.

@@ -261,6 +261,7 @@ you up to date with the multi-language support provided by Elektra.
- We now test the [Directory Value Plugin](https://www.libelektra.org/plugins/directoryvalue) with additional test data. _(René Schwaiger)_
- <<TODO>>
- The [CFramework](https://master.libelektra.org/tests/cframework) now also compares the names of meta keys. _(René Schwaiger)_
- Java test_keySetCut_shouldPass and related documentation were fixed _(Miruna Orsa)_

This comment has been minimized.

Copy link
@sanssecours

sanssecours Mar 29, 2019

Member

Please format this file with Prettier. Otherwise the test check_formatting will fail. If you want, you can also follow the instructions printed by the build server to fix the problem.

It might also make sense to surround the text “test_keySetCut_shouldPass” with back-ticks (`) to mark it as code snippet. Otherwise some Markdown parsers will assume you meant to emphasize the term “keySetCut”.

This comment has been minimized.

Copy link
@mirunix

mirunix Apr 2, 2019

Author

Thank you for the feedback, I will fix this today.

mirunix added some commits Apr 2, 2019

Merge branches 'issue/2551' and 'master' of https://github.com/miruni…
…x/libelektra into issue/2551

# Conflicts:
#	doc/news/_preparation_next_release.md
@mirunix

This comment has been minimized.

Copy link
Author

commented Apr 2, 2019

Release notes file was formatted with prettier and the test check_formatting is passing now. Please review and merge. Thank you! @markus2330

@@ -158,10 +158,8 @@ public void test_keySetAppendKeySet_shouldPass() {
public void test_keySetCut_shouldPass() {
final KeySet ks = KeySet.create(6, key, key2, key3, key4, key5, key6);
final KeySet ks2 = ks.cut(key4);
assertEquals(3, ks2.length());
assertEquals(5, ks.length());

This comment has been minimized.

Copy link
@kodebach

kodebach Apr 2, 2019

Contributor

I think you may have misunderstood, how ksCut works. It doesn't just remove the one key given as an argument. It actually moves all keys below the given key (including the given key) into a new KeySet.

So in this test case the result should be ks == { key, key2, key3 } and ks2 == { key4, key5, key6 }.

This comment has been minimized.

Copy link
@mirunix

mirunix Apr 2, 2019

Author

I asked that in the original issue #2551 and got this response:
#2551 (comment)

This comment has been minimized.

Copy link
@kodebach

kodebach Apr 2, 2019

Contributor

The javadoc was indeed wrong, and maybe the implementation is as well, but the test case was definitely correct in its original form...

If you think of all the keys as file paths, then a KeySet is the equivalent of a file system and ksCut is the equivalent of cutting a directory and pasting it into a new file system (i.e. KeySet).

So the KeySet

/some/key
/some/key/a
/some/key/b
/some/parent/key
/some/parent/key/child
/some/parent/key/child/grandchild
/some/parent/key/otherchild
/some/parent/otherkey
/the/last/key/here

when cut at the key /some/parent/key is modified to become this KeySet

/some/key
/some/key/a
/some/key/b
/some/parent/otherkey
/the/last/key/here

and the following KeySet is created and returned

/some/parent/key
/some/parent/key/child
/some/parent/key/child/grandchild
/some/parent/key/otherchild

Here is the corresponding documentation of the C function:

/**
* Cuts out a keyset at the cutpoint.
*
* Searches for the cutpoint inside the KeySet ks.
* If found it cuts out everything which is below (see keyIsBelow()) this key.
* These keys will be missing in the keyset @p ks.
* Instead, they will be moved to the returned keyset.
* If @p cutpoint is not found an empty keyset is returned and @p ks
* is not changed.
*
* The cursor will stay at the same key as it was before.
* If the cursor was inside the region of cut (moved)
* keys, the cursor will be set to the key before
* the cutpoint.
*
* If you use ksCut() on a keyset you got from kdbGet() and plan to make
* a kdbSet() later, make sure that you keep all keys that should not
* be removed permanently. You have to keep the KeySet that was returned
* and the KeySet @p ks.
*
* @par Example:
*
* You have the keyset @p ks:
* - @p system/mountpoint/interest
* - @p system/mountpoint/interest/folder
* - @p system/mountpoint/interest/folder/key1
* - @p system/mountpoint/interest/folder/key2
* - @p system/mountpoint/other/key1
*
* When you use
* @snippet ksCut.c cut
*
* Then in @p returned are:
* - @p system/mountpoint/interest
* - @p system/mountpoint/interest/folder
* - @p system/mountpoint/interest/folder/key1
* - @p system/mountpoint/interest/folder/key2
*
* And in @p ks are:
* - @p system/mountpoint/other/key1
*
* So kdbSet() permanently removes all keys below
* @p system/mountpoint/interest.
*
* @see kdbGet() for explanation why you might get more keys than you
* requested.
*
* @return a new allocated KeySet which needs to deleted with ksDel().
* The keyset consists of all keys (of the original keyset ks)
* below the cutpoint. If the key cutpoint exists, it will
* also be appended.
* @retval 0 on null pointers, no key name or allocation problems
* @param ks the keyset to cut. It will be modified by removing
* all keys below the cutpoint.
* The cutpoint itself will also be removed.
* @param cutpoint the point where to cut out the keyset
*/
KeySet * ksCut (KeySet * ks, const Key * cutpoint)


Also the test case should not only check that ks2 contains the correct keys, but also that ks doesn't contain them anymore.

This comment has been minimized.

Copy link
@mirunix

mirunix Apr 2, 2019

Author

Ok, so then I suppose that the implementation has to be fixed, because right now it's not working as described above. @markus2330 do you agree?

This comment has been minimized.

Copy link
@sanssecours

sanssecours Apr 3, 2019

Member

May I ask why you think it is not working as described above?

I executed the test for the JNA bindings in my local copy of the repository (in the master branch) using:

mkdir build
cd build
cmake -GNinja -DBINDINGS="jna" ..
ctest -V -R jna

. The last test reports:

19: -------------------------------------------------------
19:  T E S T S
19: -------------------------------------------------------
19: Running org.libelektra.KeyTest
19: Tests run: 31, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.116 sec - in org.libelektra.KeyTest
19: Running org.libelektra.KeySetTest
19: Tests run: 17, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.011 sec - in org.libelektra.KeySetTest
19:
19: Results :
19:
19: Tests run: 48, Failures: 0, Errors: 0, Skipped: 0
19:
1/1 Test #19: testjna_maven ....................   Passed    2.99 sec

The following tests passed:
	testjna_maven

100% tests passed, 0 tests failed out of 1

Label Time Summary:
bindings    =   2.99 sec*proc (1 test)
kdbtests    =   2.99 sec*proc (1 test)
memleak     =   2.99 sec*proc (1 test)

Total Test time (real) =   3.04 sec

, which seems to indicate that the test for the JNA binding succeeds.

Now let us check, if test_keySetCut_shouldPass checks the behavior of cut correctly. Currently the tests creates a key set ks that contains the following keys:

/key_test/1/key_name
/key_test/2/key_name
/key_test/3/key_name
/key_test/4/key_name
/key_test/4/key_name/1
/key_test/4/key_name/1/123

. The test then “cuts” the key set on key4 (with name /key_test/4/key_name) and stores the result in ks2. According to the documentation, after cutting the key set ks2 should contain everything that is below /key_test/4/key_name (also including the cut point /key_test/4/key_name):

/key_test/4/key_name         # key4
/key_test/4/key_name/1       # key5
/key_test/4/key_name/1/123   # key6

. This is also what the test code:

assertEquals(3, ks2.length());
assertNotNull(ks2.lookup(key4));
assertNotNull(ks2.lookup(key5));
assertNotNull(ks2.lookup(key6));

checks, as far as I can tell.

This comment has been minimized.

Copy link
@sanssecours

sanssecours Apr 3, 2019

Member

While I was fixing issue #2574, I also noticed that there seems to be something wrong with the assertNotNull test statements. At least the following assertions do not fail:

assertNotNull(ks2.lookup(key));
assertNotNull(ks2.lookup(key2));
assertNotNull(ks2.lookup(key3));

. I then added additional assertions to debug the situation:

assertEquals(3, ks.length());
assertEquals(KEY_1_NAME, ks.at(0).getName());
assertEquals(KEY_2_NAME, ks.at(1).getName());
assertEquals(KEY_3_NAME, ks.at(2).getName());

assertEquals(3, ks2.length());
assertEquals(KEY_5_NAME, ks2.at(0).getName());
assertEquals(KEY_6_NAME, ks2.at(1).getName());
// Should this not be the first key, since 
// 
// - `/key_test/4/key_name` 
// 
// is lexically smaller than
// 
// - `/key_test/4/key_name/1`, and
// - `/key_test/4/key_name/1/123`
// 
// ?
assertEquals(KEY_4_NAME, ks2.at(2).getName());

. The assertions above all succeed on my machine. Apart from the ordering issue, cut seems to work as intended.

This comment has been minimized.

Copy link
@markus2330

markus2330 Apr 3, 2019

Contributor

Thank you for this elaborate help!

@mirunix please also add these further test cases.

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.