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 Unit Test for CompressOutbox to Tempfile Creation Refactoring (SOFTWARE-5540) #176

Open
wants to merge 10 commits into
base: 2.x
Choose a base branch
from

Conversation

jeff-takaki
Copy link
Contributor

@brianhlin I have been getting AssertionError: False is not true and I think it has to do with how I am using the mock patch. I tried a few different ways of incorporating the mock with not much success. It's possible I am provisioning the test environment incorrectly??

@brianhlin brianhlin changed the title Software 5540 unittest compress outbox 1 Unittest compress outbox 1 (SOFTWARE-5540) Jul 31, 2023
@brianhlin
Copy link
Member

@jtakaki-matc I don't see anything obvious that would indicate why this failed so I would

  1. Comment out the teardown function
  2. Run this test locally and inspect the filesystem to see if the expected tarball gets created. This may be a good spot to add some changes to the actual CompressOutbox function to add debugging print() statements

@jeff-takaki jeff-takaki force-pushed the SOFTWARE-5540-unittest-compress-outbox-1 branch from 763a4ff to 46d3c94 Compare August 1, 2023 16:34
@jeff-takaki jeff-takaki marked this pull request as ready for review August 1, 2023 16:41
@jeff-takaki
Copy link
Contributor Author

@brianhlin Test seems to be working, checked locally running from 2.x branch as well.

Copy link
Member

@brianhlin brianhlin left a comment

Choose a reason for hiding this comment

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

This is looking great! One minor whitespace change and a request for additional tests.

EDIT: Could you also update the title of this PR? I don't think it summarizes ALL of the commits in the PR

test/test_sandbox_mgmt.py Outdated Show resolved Hide resolved
test/test_sandbox_mgmt.py Show resolved Hide resolved
@jeff-takaki jeff-takaki changed the title Unittest compress outbox 1 (SOFTWARE-5540) Add Unit Test for CompressOutbox to Tempfile Creation Refactoring (SOFTWARE-5540) Aug 2, 2023
@jeff-takaki
Copy link
Contributor Author

@brianhlin Added two more tests!

Copy link
Member

@brianhlin brianhlin left a comment

Choose a reason for hiding this comment

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

A couple of improvements to be made but this is looking great!

test/test_sandbox_mgmt.py Outdated Show resolved Hide resolved
test/test_sandbox_mgmt.py Outdated Show resolved Hide resolved
test/test_sandbox_mgmt.py Outdated Show resolved Hide resolved
test/test_sandbox_mgmt.py Outdated Show resolved Hide resolved
test/test_sandbox_mgmt.py Outdated Show resolved Hide resolved
test/test_sandbox_mgmt.py Outdated Show resolved Hide resolved
test/test_sandbox_mgmt.py Outdated Show resolved Hide resolved
test/test_sandbox_mgmt.py Outdated Show resolved Hide resolved
names.sort()
outfiles.sort()

self.assertListEqual(names, outfiles)
Copy link
Member

Choose a reason for hiding this comment

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

This is a good assertion! Another one would be to also check that the contents are expected

test/test_sandbox_mgmt.py Outdated Show resolved Hide resolved
jeff-takaki and others added 2 commits August 9, 2023 11:00
Co-authored-by: Brian Lin <brianhlin@gmail.com>
Make corrections to tarball file extraction and tarball location check
@jeff-takaki
Copy link
Contributor Author

@brianhlin Made a few changes per your suggestions.

Copy link
Member

@brianhlin brianhlin left a comment

Choose a reason for hiding this comment

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

Looks solid! A suggestion to improve the message upon one test failure and a slight logic issue. Pre-approving

test/test_sandbox_mgmt.py Outdated Show resolved Hide resolved
test/test_sandbox_mgmt.py Outdated Show resolved Hide resolved
Co-authored-by: Brian Lin <brianhlin@gmail.com>
get_tarball_function handles IndexError exception and fails test if no tarball is created.
test_tarball_creation and test_tarball_contents both make a call to this function
@jeff-takaki
Copy link
Contributor Author

@brianhlin Refactored!

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