integration/system: update TestInfoAPI to not use string-matching#46766
Merged
thaJeztah merged 1 commit intomoby:masterfrom Nov 3, 2023
Merged
integration/system: update TestInfoAPI to not use string-matching#46766thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah merged 1 commit intomoby:masterfrom
Conversation
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]>
rumpl
approved these changes
Nov 3, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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, andnow 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.Infostruct,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:
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
SystemTimeinto 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)