Fixes for user-tool (forward port to main of #3440) #3465
Conversation
ndokos
added
bug
Agent
tools
There was a problem hiding this comment.
I'm not sure what caused [the
test_startandtest_stop] failures, but the cause is probably the breakup of the options into separate arguments.
I couldn't find it either (I kinda got lost in the layers of mocking), but I concur with your instinct. However, the options appear to be "opt1;opt2" which doesn't seem like it would get much exercise (i.e., I would expect shlex.split() to leave it basically unchanged)...which, looking at the test code is probably a pretty Good Thing™, because it looks like the test isn't expecting the options to be split into separate arguments....
it's not clear to me why [-51 and -52] fail locally. OTOH, they do pass locally when run with
tox -- agent util-scripts. Explanations/theories/guesses are welcome.
I routinely see different behavior between my test environment and Mr. Jenkins'. I believe that those two tests actually involve multiple processes, which means that they are potentially nondeterministic not only in terms of execution order but also in terms of output capture. So, it's possible that in your environment, the TM gets to exit before the command tries to wait, and so it doesn't issue that message.
In terms of review, I found a typo in a comment and I have a couple of code suggestions...nothing that I would block the merge for.
| # Parse an entry from the output of `pbench-list-tools` above. | ||
| # The format is: "group: <group>; host: <host>; tools: <tool> <options, <tool> <options>, ... |
There was a problem hiding this comment.
The first <options is missing its closing angle bracket.
* Fixes for user-tool The tool meister was treating tools options as a concatenated string that was passed as a single argument to `pbench-install/start/stop-tools`. They are now split into separate arguments, allowing the scripts to work correctly. `pbench-postprocess-tools` was being called with empty tool options. Instead of burrowing even deeper into the filesystem to grab the options, we get rid of most of that by rewriting it to use `pbench-list-tools -g <group> -o` and parsing the output of that script. The parsing is ugly but is arguably less ugly than the file system ad-hockery that we used to go through. * Fix failures in the python tests `test_install` was using a NullObject for a logger, but the `install` method now logs its command. So we set up a basic logger to satisfy that need. Two other tests (`test_start` and `test_stop`) failed on an `is` test. We now test for equality, rather than identity and the tests pass. I'm not sure what caused these two failures, but the cause is probably the breakup of the options into separate arguments. * Fix legacy util-scripts tests Tests -51, -52 and -61 were failing because of the additional `install` logging. Tests -53, -56 and -57 now pass: the new output looks correct to me and the old output looks wrong. The util-scripts also pass in the CI container (run locally with `jenkins/run tox -- agent util-scripts`), except for -51 and -52 which fail with identical errors: - pbench-tool-meister-stop: waiting for tool-data-sink (#####) to exit The line is missing for some reason - I hope that the real Jenkins will pass them, but it's not clear to me why they fail locally. OTOH, they do pass locally when run with `tox -- agent util-scripts`. Explanations/theories/guesses are welcome. PBENCH-1172
|
This is a rebase plus a second commit to address @webbnh's review comments. |
Fixes for user-tool
The tool meister was treating tools options as a concatenated string that was passed as a single argument to
pbench-[install|start|stop]-tools. They are now split into separate arguments, allowing the scripts to work correctly.pbench-postprocess-toolswas being called with empty tool options. Instead of burrowing even deeper into the filesystem to grab the options, we get rid of most of that by rewriting it to usepbench-list-tools -g <group> -oand parsing the output of that script. The parsing is ugly but is arguably less ugly than the file system ad-hockery that we used to go through.Fix failures in the python tests
test_installwas using aNullObjectfor a logger, but theinstallmethod now logs its command. So we set up a basic logger to satisfy that need.Two other tests (
test_startandtest_stop) failed on anistest. We now test for equality, rather than identity and the tests pass. I'm not sure what caused these two failures, but the cause is probably the breakup of the options into separate arguments.Fix legacy util-scripts tests
Tests -51, -52 and -61 were failing because of the additional
installlogging.Tests -53, -56 and -57 now pass: the new output looks correct to me and the old output looks wrong.
The util-scripts also pass in the CI container (run locally with
jenkins/run tox -- agent util-scripts), except for -51 and -52 which fail with identical errors:The line is missing for some reason - I hope that the real Jenkins will pass them, but it's not clear to me why they fail locally. OTOH, they do pass locally when run with
tox -- agent util-scripts. Explanations/theories/guesses are welcome.PBENCH-1172