Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mocking-based unit tests for the stop command #68

Merged
merged 49 commits into from
Dec 23, 2024

Conversation

SimonL22
Copy link
Contributor

@SimonL22 SimonL22 commented Sep 9, 2024

No description provided.

createt automated testing
create new folder commands and set up the file for testing the command file stop
…lever/commands/test_status_execute_mocken.py

changing the path of the test_status_execute_mocken file
1. Add a test for the first part of the execute function
2. More detailed description in the comments
Fixing some pep8 warnings, shortening the code and making it more precise.
Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments.


def test_should_have_qleverfile(self):
result = StopCommand().should_have_qleverfile()
assert result
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some self.assertTrue(StopCommand().should_have_qleverfile()) opportunity here
(one line less + more important always use the same assertion mechanims consistently.

Comment on lines 31 to 49
# Parse an empty argument list to see the default
args = parser.parse_args([])

# Test that the default value for cmdline_regex is set correctly
self.assertEqual(args.cmdline_regex, "ServerMain.* -i "
"[^ ]*%%NAME%%")

# Test that the help text for cmdline_regex is correctly set
argument_help = subparser._group_actions[-2].help
self.assertEqual(argument_help, "Show only processes where "
"the command line matches this regex")

# Test that the default value for no-containers is set correctly
self.assertEqual(args.no_containers, False)

# Test that the help text for no-containers is correctly set
argument_help = subparser._group_actions[-1].help
self.assertEqual(argument_help, "Do not look for containers, "
"only for native processes")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be simpler to also mock the ArgumentParser for the testing of the add_argument_group.
Otherwise you could use implement some abstraction, that does the assertions on the actual ArgParse object, but abstracts away all the stuff with the _group_actions[-1] etc.
But maybe I am overthinking.

Comment on lines 17 to 21
# Setup args
args = MagicMock()
args.cmdline_regex = "ServerMain.* -i [^ ]*%%NAME%%"
args.name = "TestName"
args.no_containers = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think (for this whole file) we agreed to split up the execute function, to make it easier testable
(separate functions for long if or try statements etc.
I am sure, if you do this , you can simplify the tests (first change the implementations, tests should still work, then simplify the tests).

@joka921 joka921 changed the title Test stop with mocking Add mocking-based unit tests for the stop command Dec 23, 2024
@joka921 joka921 merged commit a64838e into ad-freiburg:main Dec 23, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants