Add unit tests for the ToolGroup class

[riya-17]

PBENCH-666

Adding Unit Tests for ToolGroup. Part of PbenchAgent Test Coverage

[dbutenhof]

Well this is new: black is blowing up with a stack trace because:

PermissionError: [Errno 13] Permission denied: 'agent/bench-scripts/test-bin/ansible'

I'm seeing this in multiple PRs now...

[webbnh]

My apologies for not posting these last night -- I only got part-way through the review and then got totally sidetracked by the Jenkins problem.

So, here are my comments so far...more to come

[webbnh]

This is great stuff, Riya! It just needs a little touching up and some enhancement.

• The tests should not be relying on assumptions about the environment. E.g., if they expect an environment variable to be undefined, they should remove it; and, if they expect a file or directory to not exist, they should be using mocks for the Path functions instead of assuming that "funny names" won't exist in the file system.
• I think you're missing a test case for a successful call to ToolGroup.verify_tool_group(). (Yes, the tests of the ToolGroup constructor would imply that it's working, but we should not necessarily be including verify_tool_group() within that "unit".)
• I recommend a different approach to testing the ToolGroup constructor. I think there are several scenarios that we should test, four of which would be very simple and one which would be fairly comprehensive, and I think we should integrate testing get_labels() and get_tools() into those scenarios rather than testing them separately (and, we should be testing the rest of the ToolGroup interface (i.e., specific object attributes) as well. (Also, for testing the ToolGroup constructor, I would recommend mocking verify_tool_group() -- I think that will simplify things as well as giving you better control.)

[webbnh]

Riya, this is coming along nicely! Most of my comments are bits of polishing around docstrings. However, I found one pair of assertions which look broken, and so I think you should consider a different mock for listdir() for the trigger file tests. Also, I wrote up a sample for how you might do the complex test scenario.

[webbnh]

This is looking good. Just one small point. (And, I'm still hoping that you'll try for a more comprehensive "last test". 🙂)

[riya-17]

This is looking good. Just one small point. (And, I'm still hoping that you'll try for a more comprehensive "last test". slightly_smiling_face)

Yes, I was working on that, I just updated the last test in last commit

[webbnh]

This looks really nice, Riya -- thanks for all of your effort on this!

If no one else has anything for you to change, we can skip my comments below (unless there's something there that you want to fix 😉) -- they are just polish, at this point -- you've got all the important stuff covered!

[webbnh]

Ship it!

[dbutenhof]

Looks good!

For future reference, it's best to have a TITLE and then the Jira reference in the git log message, because the Jira reference isn't as useful to a reader in figuring out what you're doing. And it'll be easier if we have a common standard format. E.g.,

TITLE

PBENCH-666

DESCRIPTION

But (assuming that the Jira integration has been jump-started and is working again 😦 ), Jira should see your reference in the title so this ought to work.

[dbutenhof]

FYI, Cobertura reports 96% of lines in tool_group.py are covered, and 93% of conditionals. That's pretty good... but analysis of the coverage map might gives you some ideas for another simple test case or two to improve that even more? (Unfortunately, the cobertura report in the PR's Jenkins artifacts complains that it "can't find the source" and won't show that detail.)

[webbnh]

This was just a rebase, right? (GitHub is claiming that there is only one commit and that the entire file is new.... 😞)

I'm assuming that you didn't actually change the code after the previous review.