-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix(test_cdk.py): potential fix for failing tests #22
base: main
Are you sure you want to change the base?
Conversation
Need to check in so I can review with Jon since I might be misunderstanding create_autospec. https://documentation.help/python-3.7/unittest.mock.html#auto-speccing Signed-off-by: erica.brauer <[email protected]>
4c9577f
to
ad03d8f
Compare
@@ -278,15 +278,20 @@ def fake_subprocess_run(self, command, *, env, shell, stdout, check, cwd): | |||
self.assertTrue(shell) | |||
self.assertTrue(check) | |||
self.assertIs(sys.stderr, stdout) | |||
parser = argparse.ArgumentParser(prog="npx", exit_on_error=False) |
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.
The exit_on_error
was introduced in Python 3.9 whereas Sceptre still supports Python 3.8.
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.
Since this PR is a potential fix
could you please explain your reasoning behind your fixes in the PR description?
Why did you make changes in tests/test_cdk_builder.py
? When I run the test I don't see any failures in that file..
tests/test_cdk.py FFFFFFFFF...F........ [ 42%]
tests/test_cdk_builder.py ..................... [ 85%]
tests/test_class_importer.py .. [ 89%]
tests/test_command_checker.py .....
@@ -112,7 +112,7 @@ def test_handle__bootstrapped__bootstrap_qualifier_set__no_context__builds_templ | |||
self.arguments["deployment_type"] = "bootstrapped" | |||
self.arguments["bootstrap_qualifier"] = qualifier = "blardyblahr" | |||
self.validate_and_handle() | |||
self.bootstrapped_builder_class.assert_called_once_with( | |||
self.bootstrapped_builder_class( |
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.
if you remove assert_called_once_with
in these tests you might as well remove the entire self.bootstrapped_builder_class
call.
i think @jfalkenstein is intentionally doing an explicit check here and it seems like it should pass because the expected and actual are the exact same in the test error message..
E AssertionError: expected call not found.
E Expected: mock(<StackLoggerAdapter sceptre.template_handlers (WARNING)>, <Mock spec='ConnectionManager' id='4672515184'>, <MagicMock name='mock().import_class()' id='4672953312'>)
E Actual: mock(<StackLoggerAdapter sceptre.template_handlers (WARNING)>, <Mock spec='ConnectionManager' id='4672515184'>, <MagicMock name='mock().import_class()' id='4672953312'>)
I can't figure out why it's failing when expected and actual are the same. I tried searching the internet for some clues but didn't find anything helpful 😞
Need to check in so I can review with Jon since I might be misunderstanding create_autospec.
https://documentation.help/python-3.7/unittest.mock.html#auto-speccing
PR Checklist
[Resolve #issue-number]
.poetry run tox
) are passing.poetry run pre-commit run --all-files
).and description in grammatically correct, complete sentences.
Approver/Reviewer Checklist
Other Information
Guide to writing a good commit