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

Added test to verify package modules are accessible as symbols #422

Closed
wants to merge 1 commit into from
Closed

Added test to verify package modules are accessible as symbols #422

wants to merge 1 commit into from

Conversation

octonawish-akcodes
Copy link
Contributor

This PR added a test to verify package modules are accessible as symbols in the kcidb directory.
This PR closes #404

@spbnick need reviews

@spbnick
Copy link
Collaborator

spbnick commented Mar 30, 2023

Please make sure CI passes before requesting reviews. Unless you need help with something. Thank you ❤️

@octonawish-akcodes
Copy link
Contributor Author

@spbnick Have a look now?

Copy link
Collaborator

@spbnick spbnick left a comment

Choose a reason for hiding this comment

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

Nice start, Abhishek. I have a few questions inline, but the main thing is that this is not recursive and only checks two levels. You can make this recursive, if you define a function inside the test, which accepts a package, checks its contents, and calls itself on each package inside the supplied package. Then you can call this function (let's call it check_package for this example) from the test function like this: check_package(kcidb).

test_kcidb.py Outdated Show resolved Hide resolved
test_kcidb.py Outdated Show resolved Hide resolved
test_kcidb.py Outdated Show resolved Hide resolved
test_kcidb.py Outdated Show resolved Hide resolved
@spbnick
Copy link
Collaborator

spbnick commented Mar 30, 2023

Also, it's strange that this doesn't fail for any of the packages you managed to check here 🤔
See if you can make it actually catch a missing module symbol by removing one from a package.

Copy link
Collaborator

@spbnick spbnick left a comment

Choose a reason for hiding this comment

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

Thank you, @octonawish-akcodes! This is pretty good already. I left you a couple requests inline.

Otherwise, have you tested this? E.g. by removing a symbol for a submodule, or adding a (dummy) submodule without adding the symbol?

test_kcidb.py Outdated
module_fullname = f"{package.__name__}.{module_name}"
module = importlib.import_module(module_fullname)
assert hasattr(package, module_name)
assert isinstance(getattr(package, module_name), type(module))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you check that the actual symbol is the module? E.g. like this:

assert getattr(package, module_name, None) is module

This would replace both of your assertions.

It would help if you added a comprehensive message for the assertion failure something along the lines of "Package X is missing the symbol for submodule Y".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried everything but its passing on every stage sir

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you figure out why, and what the solution should be? This test would be useless otherwise.

test_kcidb.py Outdated Show resolved Hide resolved
@octonawish-akcodes octonawish-akcodes closed this by deleting the head repository Jun 30, 2023
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.

Test that package modules are accessible as symbols
2 participants