[new-backend] Fix unit tests

eiskasten commented 1 year ago

Lots of tests currently fail in the master branch (c56018228d913a67de3b37e79cd51a652c5285f1).

Expected Result

All tests should pass

Actual Result

The following tests are failing:

[x] 19 - testlib_notification
[x] 36 - test_kdb.py
[x] 37 - test_gopts.py
[x] 41 - test_tools_plugindatabase.py
[x] 46 - test_kdb.lua
[x] 47 - test_gopts.lua
[ ] 69 - testshell_markdown_base64
[x] 73 - testshell_markdown_blockresolver
[x] 82 - testshell_markdown_cpptemplate
[ ] 105 - testshell_markdown_hosts
[x] 138 - testshell_markdown_multifile
[ ] 139 - testmod_python
[x] 160 - testshell_markdown_shell
[x] 192 - testscr_check_doc (@kodebach #4484)
[x] 196 - testscr_check_gen
[x] 197 - testscr_check_get_set
[x] 200 - testscr_check_kdb_internal_suite
[x] 205 - testscr_check_plugins (@kodebach #4484)
[x] 209 - testscr_check_resolver
[x] 234 - testshell_markdown_tutorial_crypto
[x] 257 - test_mount (@Eiskasten #4507)

The following tests are (partially) disabled:

[ ] 2 - testtool_backend
[ ] 156 - testmod_resolver
[ ] 239 - testshell_markdown_cache_shelltests

The state of the following tests is currently unknown to me:

[ ] 33 - testjna_gradle

I will add further logs and information in a comment per testsuite. This should make the discussion make much more clear. If a testsuite is fixed, it can be checked in this list.

System Information

Elektra: new-backend
Operating System: Debian Sid Container

markus2330 commented 1 year ago

@Eiskasten Thank you very much for kicking off this issue :rocket: :revolving_hearts:

@atmaxinger can you give some input about the extra tests that failed after merging #4471?

atmaxinger commented 1 year ago

testkdb_error
testkdb_nested
testkdb_simple

The problem lies in goptsActive. Previously, it was checked like this:

bool goptsActive = handle->globalPlugins[PROCGETSTORAGE][MAXONCE] != NULL;

Which doesn't really tell you if gopts is really activated, just if there is some plugin loaded there. Spoiler: the list plugin is. This with the fact that both backends are in the proc:/ namespaces led to the following block executing:

https://github.com/ElektraInitiative/libelektra/blob/3e9f5ed10725627405fb36c7a1ea33fa7eb67a1e/src/libs/elektra/kdb.c#L1780-L1794

So this is more or less a bug/works-by-accident behaviour of the new-backend branch.

kodebach commented 1 year ago

Yes, the goptsActive check wasn't correct. But as I said in https://github.com/ElektraInitiative/libelektra/pull/4471#issuecomment-1255095972 the failing tests have existed in this form since before gopts (and PROCGETSTORAGE) was a thing.

Also AFAICT 0 is the correct return value here. All these tests set up temporary mountpoints without creating the storage files. Therefore the resolver will respond with ELEKTRA_PLUGIN_STATUS_NO_UPDATE, which means there are no backends that need to be read.

What this if should check is that either we have no backend left, or the only backend left is the fake backend for gopts. Because those are the cases, where we don't need to do anything. Maybe it is also possible to move adding the fake backend to after this if. Then it would just be if (ksGetSize (backends) == 0).

kodebach commented 1 year ago

@Eiskasten there are also quite a few tests in new-backend that are commented out or disabled.

Some of them are tests for old data structures that aren't used anymore. In those cases, there might not yet be tests for the new data structures.

https://github.com/ElektraInitiative/libelektra/blob/fac849659be65b3021ded570dcf33e2edb95c407/tests/ctest/test_mount.c#L15-L17

Other tests, check behaviour that has changed a bit

https://github.com/ElektraInitiative/libelektra/blob/fac849659be65b3021ded570dcf33e2edb95c407/tests/kdb/testkdb_simple.cpp#L87-L89

atmaxinger commented 1 year ago

@kodebach do we actually have tests for the spec plugin testing the bahaviour as global plugin that I can repurpose for the hooks? The two I found (test_spec and testmod_spec) all pass. But there must be some tests in elektra that actually test the behaviour of kdbGet and kdbSet with spec, right?

kodebach commented 1 year ago

test_spec AFAICT that's actually a test for ksLookup (more specifically the elektraLookupBySpec part).

testmod_spec that should be the main test for the actual functionality of spec.

Tests for how kdbGet/kdbSet spec don't really exist AFAIK. But all the testkdb_* do full kdbGet and kdbSet calls with really mountpoints. Some of them may also rely on spec (I haven't checked). There is also testscr_check_gen which does a full highlevel API code-generation and and application test compile and test run. That one definitely needs spec to work.

If you want tests for just the hook, you should probably copy what you did for gopts.

kodebach commented 1 year ago

@Eiskasten I'll fix testscr_check_doc and testscr_check_plugins in #4484

eiskasten commented 1 year ago

@Eiskasten there are also quite a few tests in new-backend that are commented out or disabled.

Some of them are tests for old data structures that aren't used anymore. In those cases, there might not yet be tests for the new data structures.

https://github.com/ElektraInitiative/libelektra/blob/fac849659be65b3021ded570dcf33e2edb95c407/tests/ctest/test_mount.c#L15-L17

Other tests, check behaviour that has changed a bit

https://github.com/ElektraInitiative/libelektra/blob/fac849659be65b3021ded570dcf33e2edb95c407/tests/kdb/testkdb_simple.cpp#L87-L89

Ok, thank you, I will have a look on them.

eiskasten commented 1 year ago

Some of them are tests for old data structures that aren't used anymore. In those cases, there might not yet be tests for the new data structures.

https://github.com/ElektraInitiative/libelektra/blob/fac849659be65b3021ded570dcf33e2edb95c407/tests/ctest/test_mount.c#L15-L17

Do you suggest removing them? I tried to run them but they are not compiling anymore. I commented the non-compiling parts but they running them still fails due to substantial changes in the backend.

kodebach commented 1 year ago

Yes, you can remove all tests that are related to split, trie or one of the functions in src/libs/elektra/mount.c. In many cases the functions thy test still exist (e.g. all the ones in mount.c), but aren't used anymore. Those functions can also be removed.

Instead of these tests, we now need to test the (non-static) backends* functions in src/libs/elektra/split.c and the elektraMountpointsParse function in src/libs/elektra/kdb.c. I haven't documented this new data structure yet, but the basic this:

There is now a KeySet * backends in struct _KDB.
The keys in this keyset are the parent keys of all mountpoints.
The values of these keys are struct _BackendDatas, which contain the data for a backend:
- Plugin * backend is the backend plugin for the mountpoint
- KeySet * definition is the definition for the mountpoint, i.e. the keys from system:/elektra/mountpoints/<mp>/definition but renamed to be below system:/
- KeySet * plugins are the plugins for this mountpoint. This is based on the keys from system:/elektra/mountpoints/<mp>/plugins renamed to be below system:/. But the data is not simply copied. Instead the keys below system:/elektra/mountpoints/<mp>/plugins/<ref> are used to open the plugin defined by them and the resulting Plugin * is stored in the key system:/<ref>. All other keys are removed.
- bool initialized is set to true when the init function of the backend plugin has been called.
- bool keyNeedsSync is set by backendsDivide to indicate whether the backend contains at least one key with the KEY_FLAG_SYNC flag set
- size_t getSize set by backendsMerge to the number of keys in the backend
- KeySet * keys the keys in the backend. backendsDivide assigns these keys by dividing a KeySet * to the backends, and backendsMerge combines the keys from all backends back into a single KeySet *.

The other functions should do:

elektraMountpointsParse parses the keys from system:/elektra/mountpoints and returns KeySet * with struct _BackendDatas as described above.
backendsForParentKey takes a KeySet * with backends and finds the ones relevant to a given parent key, i.e. the ones that must be called for this parent key
backendsFindParent takes KeySet * with backends and finds the mountpoint of a given Key *

Note backendsMerge currently also clears KEY_FLAG_SYNC. This will probably change, see https://github.com/ElektraInitiative/libelektra/pull/4504#discussion_r987269759 (bottom half). So tests probably shouldn't check that yet.

For backendsForParentKey and backendsFindParent only the key names are relevant. In normal operation the values of the keys would be struct _BackendDatas, but in the tests they can just be empty or simple strings. Similarly, backendsDivide and backendsMerge can use simpler input in tests. They do need actual struct _BackendData key values, but the Plugin * backend, KeySet * plugins and KeySet * definition can simply be NULL. Only KeySet * keys must be allocated and the other fields, are there anyway.

Re priorities: Adding tests for backendsForParentKey and backendsFindParent is probably most important and easiest. Next we should add tests for backendsDivide and backendsMerge. There is already a very basic test_backendsDivide in test_split.c, but we should have more thorough tests. Tests for elektraMountpointsParse should be added last, since they are the most complex to write.

markus2330 commented 1 year ago

@Eiskasten @kodebach What can we realistically do here before the next release hopefully coming soon (January)?

kodebach commented 1 year ago

I personally, won't have the time. Since I'm still focusing on the decisions. When those are finished (whenever that may be), I might find time. However, there is not that much left, so I believe somebody else could still make progress in January (or at least Feburary).

Regarding tests, I found these somewhat important FIXMEs:

test_getenv is disabled
Rust binding has all tests for KDB disabled
Tests for these plugins are disabled: conditionals, multifile, resolver (only some)
testscr_check_resolver is partially disabled with a reference to wresolver

There are also a few less important FIXMEs/TODOs:

testtool_backend (can be ignored, mounting code will be replaced anyway)
The libelektra-kdb functions ensureContract() and elektraMountpointsParse() and the backend plugin don't have any unit tests at all. They are only tested indirectly via the kdb and shell tests.
Tests for cache are disabled, since it is not implemented yet.
test_backends only contains a single test for backendsDivide in a single configuration. There could be more unit tests for all the backends* functions. But all of them are indirectly tested via the kdb and shell tests.

markus2330 commented 1 year ago

@Eiskasten can you please update the top-post for which tests still need fixes?

eiskasten commented 1 year ago

I have updated the state of the tests

markus2330 commented 1 year ago

@Eiskasten is there something that can go into the next release?

eiskasten commented 1 year ago

Nothing I know about from

github-actions[bot] commented 1 month ago

I mark this stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping by writing a message here or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

github-actions[bot] commented 1 month ago

I closed this now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

ElektraInitiative / libelektra