-
Notifications
You must be signed in to change notification settings - Fork 302
functional: more fleetctl unit action tests #1544
functional: more fleetctl unit action tests #1544
Conversation
38ec1aa
to
0313fa8
Compare
t.Fatalf("Unable to submit fleet unit: %v", err) | ||
} | ||
stdout, _, err := cluster.Fleetctl(m, "list-units", "--no-legend") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dongsupark thanks for the patches!
Hmm something is wrong here, my understanding with fleet is submit only sends the units into the cluster, so it will show up when you do "fleetctl list-unit-files" but not with "fleetctl list-units" since the unit is not active.
Was this test wrong from the beginning ? we should not check the number of printed lines, we should just grep for the name of the unit, thoughts ?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't "wrong", just not specific enough, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonboulle yes! by "wrong" I'm only referring to the functional tests and that perhaps there is a bug somewhere that allowed it to pass. For fleetctl commands they are clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonboulle yes not specific enough ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dongsu capturing the context, maybe we need a function runCheckListUnits()
, runCheckListUnitFiles()
and if you want to grep submitted units since they won't show up in other commands and you are not sure about the number of returned units... then make runGetListUnitFiles() return the units and grep it into the caller ? just quick thoughts.
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonboulle no, it was indeed completely wrong: list-units
returns an empty string; but that's still counts as one line, so the test "passed"...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that most of the existing tests only check the number of units, except for cases where there is actually a possibility for ambiguity -- so checking only the count would be fine here as well. It just needs to be fixed to use list-unit-files
; and to properly check for a non-empty string...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just saw that one of the existing tests in this very file (namely TestUnitRunnable()
) actually test both the count and the name -- so feel free to copy that if you really think it's worthwhile...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it was indeed completely wrong: list-units returns an empty string; but that's still counts as one line, so the test "passed"...
Yes, that was exactly what I wanted to say. That's why I said yesterday that this part has been wrong since the beginning. I think the underlying reason was that len(strings.Split("", "\n"))
returns 1, not 0 as expected.
So my solution to get it working was adding additional conditions like "expectedUnits == 0 && len(outListUnitFiles) == 0
" in each check function. Still somehow nasty, but it's working for now. Afterwards, we should do some code auditing in the entire tree to find such errors. But anyway that would be out of the scope of this PR.
0313fa8
to
e6ba65f
Compare
Updated.
|
// submit a unit and assert it shows up | ||
if _, _, err := cluster.Fleetctl(m, "submit", "fixtures/units/hello.service"); err != nil { | ||
_, _, err = cluster.Fleetctl(m, "submit", unitFile) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change the syntax here? There is no reason to have the assignment outside the if
.
So a couple of changes then maybe merge this one ? |
e6ba65f
to
28ebafd
Compare
Updated.
I assume this PR should be merged prior to the existing #1509. |
t.Fatalf("Failed to destroy unit: %v", err) | ||
} | ||
stdout, _, err = cluster.Fleetctl(m, "list-units", "--no-legend") | ||
// wait until the unit gets submitted up to 15 seconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment is still wrong here...
In addition to the existing UnitState, define UnitFileState to parse output of fleetctl list-unit-files, as well as its parse function ParseUnitFileStates(). The existing ParseUnitStates() parses Name, State, and Machine, including specific handling of machine strings. In contrast, ParseUnitFileStates() simply parses 3 fields: Name, DesiredState, and State.
…Files WaitForNUnits() runs fleetctl list-units to get a map of []UnitState (unit, active, machine). This operation should be called after fleetctl load or start. WaitForNUnitFiles() runs fleetctl list-unit-files to get a map of []UnitFileState (unit, desiredstate, state). This operation should be called after fleetctl submit.
A new functional test TestUnitCat simply tests if "fleetctl cat" works.
TestUnitStatus simply checks if "fleetctl status hello.service" works.
Improve TestUnitSubmit() like the following: * use list-unit-files instead of list-units, to correctly verify submitted units. * check that the output of list-unit-files contains the correct unit name, with help of a new helper cluster.WaitForNUnitFiles. That way the whole functional test could become less racy.
A new test TestUnitLoad verifies that "fleetctl {load,unload}" correctly works: load -> list-units -> unload -> list-units -> load
28ebafd
to
7bcdb51
Compare
Done.
|
lgtm! thanks all. |
In order to make functional tests cover more fleetctl commands, do the following:
util.UnitFileState
for handling output of list-unit-filesWaitForNUnits
andWaitForNUnitFiles
, wrappers based onutil.WaitForState
.TestUnitCat
for "fleetctl cat
".TestUnitStatus
for "fleetctl status
".TestUnitSubmit
.TestUnitLoad
forfleetctl load
.