CLI unit tests

[joseivanlopez]

Problem

Snapper offers several CLI commands. Recently, some of that commands have been extended to support machine-readable output, see https://github.com/openSUSE/snapper/pull/494. But the code lacks of unit tests for the CLI commands.

Some infrastructure should be provided to allow CLI unit tests.

Proposal

Snapper client is able to use two different methods to communicate with snapper. It can use either snapper library directly or dbus. Some proxy classes allows to snapper client to perform the communication transparently, independently on dbus is used or not. For that, each proxy class has two different implementations (or derived classes). For example, for the class ProxySnappers there are ProxySnappersLib and ProxySnappersDbus implementations.

To allow unit-testing CLI methods, we could add a new proxy implementation for testing specifically, e.g. ProxySnappersTest. That testing implementation would probe data from a file instead of asking to snapper lib/dbus.

For actions that perform system changes (e.g. creating an snapshot), the testing proxy classes could register the action call (e.g., using an specific testing log). Then, to know if the action was performed, unit tests could simply check if the expected action was registered. With that system, unit tests are not checking that snapper actually performs the action correctly (e.g., that an snapshot was created), but at least we can check that a CLI command performs the expected action.

For probing data (e.g., the list of snapshots) we need to provide a file with the data. The new testing proxy classes would use that data file. Due to rapidjson dependency was already added to snapper (see https://github.com/openSUSE/snapper/pull/501), it makes sense to use json file for probing data. For example, we might use something like the following:

{
    snappers: [
        {
            config: {
                name: value,
                values: {
                    key1: value1,
                    key2: value2
                }
            },
            quota: value,
            free_space_data: value,
            snapshots: [
                {
                    type: value,
                    number: value,
                    date: value,
                    uid: value,
                    pre_number: value,
                    description: value,
                    cleanup: value,
                    current: value,
                    used_space: value
                },
                { ... },
                { ... }
            ]
        },
        { ... },
        { ... }
    ]
}

Example

Unit tests would use a ProxySnappersTest to create a ProxySnappers object:

ProxySnappers snappers(new ProxySnappersTest.new("file.json"))

For commands that only show data, unit tests simply check the command output:

result = command.run;

expected = "a,b\n1,2\n";

BOOST_CHECK_EQUAL(result, expected);

For commands that modifies the system, unit tests check that the correct command would be performed:

command.run;

found = ActionsRegister.find({"createSingleSnapshot", "10", "read_only: false, empty: true"})

BOOST_CHECK_EQUAL(found, true);

[aschnell]

Comments about the proposal:

1. It is not clear what result is. Make sure the tests can also check for errors (stderr and exceptions).

2. The order of actions is important to unit tests should check all registered actions including the ordering instead of just looking for some action.

3. Not all snapper commands use the Proxy classes for everything. E.g. diff, undochange and rollback use libsnapper or other code directly. This is done on purpose for security reasons (the less snapperd can do the better since it can have more permissions than snapper itself).

[joseivanlopez]

Comments about the proposal:

1. It is not clear what `result` is. Make sure the tests can also check for errors (stderr and exceptions).

Not decided yet. Maybe, we could pass the stream buffer to the command (cout by default) or we could intersect the cout buffer in the unit tests.

2. The order of actions is important to unit tests should check all registered actions including the ordering instead of just looking for some action.

Ok.

3. Not all snapper commands use the Proxy classes for everything. E.g. diff, undochange and rollback use libsnapper or other code directly. This is done on purpose for security reasons (the less snapperd can do the better since it can have more permissions than snapper itself).

Yes, you are right. Having a look to the commands, I see the following ones are using some libsnapper class directly (without a Proxy): create_config, status, diff, undo, rollback, xa_diff.

And the following commands always use Proxy classes: list_configs, delete_config, get_config, set_config, list, create, modify, delete, mount, umount, set_quota, cleanup, debug.

I was discussing it with @ancorgs and we decided to start with the commands using Proxy classes. We can consider it as a first iteration. Then, if we are happy with the result after this first iteration, we can evaluate how to adapt the code of the rest of commands to inject mocking classes.

Anyway, I am open to consider other kind of approaches for unit-testing the CLI commands. @aschnell would you suggest another option?

[aschnell]

It should be possible to add some test-mode to the library. That would 1. allow more tests (not only things using the Proxy classes), 2. also allow to test the client and server at once (although the setup would be more complicated) and 3. also allow to test other clients.