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

test: switch test frameworks #266

Merged
merged 2 commits into from
Apr 8, 2024
Merged

test: switch test frameworks #266

merged 2 commits into from
Apr 8, 2024

Conversation

cesarcoatl
Copy link
Member

from pytest to unittest
get system modules dynamically

from pytest to unittest
get system modules dynamically
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @cesarcoatl - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟡 Testing: 3 issues found
  • 🟢 Complexity: all looks good
  • 🟢 Docstrings: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

locals()[test_name] = _create_import_test(module_name)
_cleanup()

def assertModuleAvailable(self, module_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (testing): Consider adding a test for dynamic module discovery failure.

While the current tests ensure that expected modules can be imported, it would be beneficial to also test the behavior when a module cannot be discovered dynamically. This could help in ensuring the robustness of the dynamic discovery mechanism.

Suggested change
def assertModuleAvailable(self, module_name):
def assertModuleAvailable(self, module_name):
try:
importlib.import_module(module_name)
except ImportError:
self.fail("Failed to import module: {}".format(module_name))
def test_module_not_found_behavior(self):
with self.assertRaises(ImportError):
importlib.import_module("non_existent_module_name")

return _MODULES


class TestPackageImports(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (testing): Ensure cleanup is called after each test to prevent state leakage.

The _cleanup function is called at the end of the module discovery process, which might not guarantee its execution after each test case. Consider using tearDown or tearDownClass methods to ensure that _cleanup is executed reliably after each test or after all tests in the class, respectively.


for module_name in modules_to_test:
test_name = "test_import_{}".format(module_name)
locals()[test_name] = _create_import_test(module_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (testing): Consider a more explicit approach than using locals() for test case generation.

Using locals() to dynamically generate test cases can make the code harder to understand and debug. A more explicit approach, such as adding test methods directly to the TestPackageImports class or using a test case generation library, might improve readability and maintainability.

Suggested change
locals()[test_name] = _create_import_test(module_name)
def _generate_test_cases():
for module_name in modules_to_test:
test_name = "test_import_{}".format(module_name)
def test(self, module_name=module_name):
self.assertModuleAvailable(module_name)
setattr(TestPackageImports, test_name, test)
_generate_test_cases()

Comment on lines 26 to 30
_module = "{}.{}".format(package, name)
_MODULES.append(_module)
_discover_modules("{}/{}".format(package, name))
else:
_module = "{}.{}".format(package, name)
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (code-quality): Hoist repeated code outside conditional statement (hoist-statement-from-if)

@cesarcoatl cesarcoatl merged commit b52c6a4 into main Apr 8, 2024
3 checks passed
@cesarcoatl cesarcoatl deleted the test/unitttest branch April 8, 2024 20:46
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.

1 participant