Fixes for pbench-clear-tools potholes

[npalaska]

Fixes issue #2337

• Added support for comma-separated lists of --name and --group option on pbench-clear-tools command.
• Removes the tool group directory after a user clears all the tools from the group

[webbnh]

This change looks good. I've got a few comments (😱), most of which are small stuff and nits. However, I think you need to rework the support for the "Tool not found" message, and I think you removed a FIXME without fixing it. 🙂 Also, we should discuss whether to keep all the assertions in the unit tests (e.g., as "specification") or whether we should include just the ones that aren't redundant.

[webbnh]

Looks better. I have a few more nits and suggestions.

[npalaska]

Hey @riya-17 I have no idea how Github decided to remove you from my reviewers when I clicked the re-review button for Dave and Webb (I might have clicked something wrong). Anyway added you back.

[webbnh]

This looks generally good, but I got kind of lost trying to review the test cases.

I'm wondering if you would be amenable to replacing all or most of the cases (particularly the new ones) with either one or two parameterized tests. I believe that we should be able to capture and test all the various combinations using a single test (with parameterized expected results) or with two tests (one for successes and one for errors). I think this will ultimately be simpler to understand, or, at least, easier to see what we cover.

If we parameterize groups, remotes, and tools, and have the tests omit the switches when the values are empty, then I think we can test all the combinations in a single test. If we have a fixture which sets up the configuration to test against, then we can vary what the tool attempts, which I think will be a better way to exercise the CUT. And, for the failure case, we can request items which are not created by the fixture, and we can easily try them in various positions in the list.

If that seems like too much to take on, let me know, and I'll try to give the rest of the test cases a proper review.

[webbnh]

We need some clarity into the test configuration which these tests are running the tool against. As I comment below, setting it up to be table-driven would suffice; otherwise, we need a good docstring on the fixture.

I'm confused about how the test is validating the cases where we omit switches.

And, I have a few smaller suggestions.

Also, we still need to get rid of the is True and is False from the assert statements.

[npalaska]

OK, how about this: the current code already uses a fairly heavy stub for command option parsing -- it would be straightforward to detect the blank entries there and fail the command before it starts. That way we get the "user validation" that I want, and we avoid the ambiguous result that Dave dislikes. --Webb

I don't consider failing up front in parameter parsing due to the commas an ideal solution, but it's better than the current state, and, more importantly, I'd accept it since overall I don't consider the behavior of this corner case sufficiently important to argue about any further. --Dave

Alright, so is the resolution of this discussion is to fail and terminate (without doing anything) by inspecting the list for an empty element before we start the operation?

Also for the "wrong name" option do we want to keep our approach as it is right now (we skip the wrong name parameter but raise an error value).

I feel we can always revisit the script in future if we need to update the approach or if the users of the pbench agent weigh their preferences of handling it some other way.

[webbnh]

I don't consider failing up front in parameter parsing due to the commas an ideal solution, but it's better than the current state, and, more importantly, I'd accept it since overall I don't consider the behavior of this corner case sufficiently important to argue about any further. --Dave

Alright, so is the resolution of this discussion is to fail and terminate (without doing anything) by inspecting the list for an empty element before we start the operation?

That satisfies me and it will earn Dave's acceptance, so, yeah, let's do that. 😀

Also for the "wrong name" option do we want to keep our approach as it is right now (we skip the wrong name parameter but raise an error value).

I would vote yes on that, as I think doing anything more comprehensive will increase cost without producing sufficient value for it.

I feel we can always revisit the script in future if we need to update the approach or if the users of the pbench agent weigh their preferences of handling it some other way.

I agree, with one caveat: we don't want a future change in our behavior to break then-existent practice. So, for instance, if we reject spurious commas now, we could add tolerance for them later; however, if we tolerate them now, then later we cannot tighten up the interface without potentially breaking someone. But, we could potentially alter how we handle the bad-option-value case in the future (although, we would have to give it some thought).

[webbnh]

This looks good to me...just some code quality suggestions.

[webbnh]

👍