Skip to content

integration/system: update TestInfoAPI to not use string-matching#46766

Merged
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:fix_TestInfoAPI
Nov 3, 2023
Merged

integration/system: update TestInfoAPI to not use string-matching#46766
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:fix_TestInfoAPI

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

integration/system: update TestInfoAPI to not use string-matching

This test was rewritten from an integration-cli test in commit
68d9bee, and originally implemented in
f4942ed, which rewrote it from a unit-
test to an integration test.

Originally, it would check for the raw JSON response from the daemon, and
check for individual fields to be present in the output, but after commit
0fd5a65 (#33534), client.Info() was used, and
now the response is unmarshalled into a system.Info.

The remainder of the test remained the same in that rewrite, and as a
result were were now effectively testing if a system.Info struct,
when marshalled as JSON would show all the fields (surprise: it does).

TL;DR; the test would even pass with an empty system.Info{} struct,
which didn't provide much coverage, as it passed without a daemon:

func TestInfoAPI(t *testing.T) {
    // always shown fields
    stringsToCheck := []string{
        "ID",
        "Containers",
        "ContainersRunning",
        "ContainersPaused",
        "ContainersStopped",
        "Images",
        "LoggingDriver",
        "OperatingSystem",
        "NCPU",
        "OSType",
        "Architecture",
        "MemTotal",
        "KernelVersion",
        "Driver",
        "ServerVersion",
        "SecurityOptions",
    }

    out := fmt.Sprintf("%+v", system.Info{})
    for _, linePrefix := range stringsToCheck {
        assert.Check(t, is.Contains(out, linePrefix))
    }
}

This patch makes the test slightly better by checking if the fields
are non-empty. More work is needed on this test though; currently it
uses the (already running) daemon, so it's hard to check for specific
fields to be correct (withouth knowing state of the daemon), but it's
not unlikely that other tests (partially) cover some of that. A TODO
comment was added to look into that (we should probably combine some
tests to prevent overlap, and make it easier to spot "gaps" as well).

While working on this, also moving the SystemTime into this test,
because that field is (no longer) dependent on "debug" state

(It is was actually this change that led me down this rabbit-hole)

                     ()_()
                     (-.-)
                    '(")(")'

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

This test was rewritten from an integration-cli test in commit
68d9bee, and originally implemented in
f4942ed, which rewrote it from a unit-
test to an integration test.

Originally, it would check for the raw JSON response from the daemon, and
check for individual fields to be present in the output, but after commit
0fd5a65, `client.Info()` was used, and
now the response is unmarshalled into a `system.Info`.

The remainder of the test remained the same in that rewrite, and as a
result were were now effectively testing if a `system.Info` struct,
when marshalled as JSON would show all the fields (surprise: it does).

TL;DR; the test would even pass with an empty `system.Info{}` struct,
which didn't provide much coverage, as it passed without a daemon:

    func TestInfoAPI(t *testing.T) {
        // always shown fields
        stringsToCheck := []string{
            "ID",
            "Containers",
            "ContainersRunning",
            "ContainersPaused",
            "ContainersStopped",
            "Images",
            "LoggingDriver",
            "OperatingSystem",
            "NCPU",
            "OSType",
            "Architecture",
            "MemTotal",
            "KernelVersion",
            "Driver",
            "ServerVersion",
            "SecurityOptions",
        }

        out := fmt.Sprintf("%+v", system.Info{})
        for _, linePrefix := range stringsToCheck {
            assert.Check(t, is.Contains(out, linePrefix))
        }
    }

This patch makes the test _slightly_ better by checking if the fields
are non-empty. More work is needed on this test though; currently it
uses the (already running) daemon, so it's hard to check for specific
fields to be correct (withouth knowing state of the daemon), but it's
not unlikely that other tests (partially) cover some of that. A TODO
comment was added to look into that (we should probably combine some
tests to prevent overlap, and make it easier to spot "gaps" as well).

While working on this, also moving the `SystemTime` into this test,
because that field is (no longer) dependent on "debug" state

(It is was actually this change that led me down this rabbit-hole)

                         ()_()
                         (-.-)
                        '(")(")'

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah added this to the 25.0.0 milestone Nov 3, 2023
@thaJeztah thaJeztah merged commit 39393f6 into moby:master Nov 3, 2023
@thaJeztah thaJeztah deleted the fix_TestInfoAPI branch November 3, 2023 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants