[bug] AAA tests do not handle mock DB connection correctly

[anders-nexthop]

Many test cases in sonic-utilities/tests/aaa_test.py and sonic-utilities/tests/radius_test.py are handling mock database connections incorrectly. The state configured by AAA config commands is not the same state used by the AAA show commands that are being used as validation. Instead, desired state is manually fed into AAA show commands to produce the desired results. This is a clear break from the prevalent pattern in sonic-utilities/tests in which config commands are verified with their accompanying show commands.

THE DETAILS

ConfigDBConnector is built so that each instance will connect to the DB separately; users can instantiate a single ConfigDBConnector and use it indefinitely, or create a new one every time a DB operation is required. Config commands are each run as a separate process, so each command will have a different ConfigDBConnector. In sonic-utilities/config/main.py, the ConfigDBConnector is instantiated at a broad command level and then passed down to subcommands as needed (using the pass_db decorator). However, in sonic-utilities/config/aaa.py, no ConfigDBConnector is passed in, and each command creates its own link to the DB. This works fine in the actual production system, since each ConfigDBConnector ends up connecting to the same DB.

The pytest framework used for sonic-utilities breadth tests does not have an actual DB instantiated at all. Instead, sonic-utilities/tests/mock_files/dbconnector.py is imported at the beginning of the test. That file defines a class SwssSyncClient which mocks RedisDB. Each instance of SwssSyncClient is unique, and will have it's own mocked versions of Sonic tables. For most config commands this works perfectly fine; because the top-level command is responsible for creating the connection, subcommands can easily be tested by creating a ConfigDBConnector connected to a mock SwssSyncClient and then passing that same ConfigDBConnector into each config command in turn. Show commands are used to validate the config changes, and since these show commands are passed the same ConfigDBConnector object, they work as expected.

In AAA tests, however, this does not work the same way. There is no way to pass a ConfigDBConnector object into AAA commands, so instead each command will make its own ConfigDBConnector, and each of those will have a distinct SwssSyncClient. Both of those objects will only last for the duration of the command, and no outside state will be set. The existing AAA tests have worked around this issue by running the config command, then creating a new ConfigDBConnector and manually adding whatever the config command should have added, then running the show command and asserting that the output matches what is expected. Here's an example from sonic-utilites/tests/aaa_test.py:

def test_config_aaa_radius(self, get_cmd_module):
    (config, show) = get_cmd_module
    runner = CliRunner()
    db = Db()
    db.cfgdb.delete_table("AAA")

    result = runner.invoke(config.config.commands["aaa"],\
                           ["authentication", "login", "radius"], obj=db)
    print(result.exit_code)
    print(result.output)
    assert result.exit_code == 0
    assert result.output == config_aaa_empty_output

    db.cfgdb.mod_entry("AAA", "authentication", {'login' : 'radius'})

    result = runner.invoke(show.cli.commands["aaa"], [], obj=db)
    assert result.exit_code == 0
    assert result.output == show_aaa_radius_output

    result = runner.invoke(config.config.commands["aaa"],\
                           ["authentication", "login", "default"], obj=db)
    print(result.exit_code)
    print(result.output)
    assert result.exit_code == 0
    assert result.output == config_aaa_empty_output

    db.cfgdb.delete_table("AAA")

    result = runner.invoke(show.cli.commands["aaa"], [], obj=db)
    assert result.exit_code == 0
    assert result.output == show_aaa_default_output

This follows the standard approach for sonic-utilities tests, dutifully passing in the created 'db' object. But the config command ignores that object, writes to its own short-lived mock db, and returns. The test is forced to make calls to mod_entry and delete_entry
to actually set the correct table state. This means that the only things being tested for AAA config commands are:

• that the return code is 0, meaning that the command syntax is correct and the command was accepted
• that the command returned no output, which is expected for config commands
  That's it. There is no validation of the actual function of the command. While it's possible that the test writers are intentionally only wanting to test the command syntax, it seems extremely unlikely given that the rest of the tests in sonic-utilities lean heavily on this pattern to test config command internals.

Either the test infrastructure must be modified to match the behavior of the underlying implementation when handling multiple mocked db connections, or the AAA config commands need to be changed to follow the same style used in other cli commands, where the database connection is passed down from the top-level config command.

Regardless of which approach is taken, the manual workaround must also be removed from all test cases where it is present, and any lurking errors resulting from the test gap will need to be addressed.