-
Notifications
You must be signed in to change notification settings - Fork 906
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
Refactor Jupyter magic unit tests #3599
Conversation
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
…kedro-org/kedro into noklam/refactor-jupyter-magic-unit-3585 Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
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.
This is so much easier to follow! 🌟
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.
This is much cleaner and easier to read, awesome work @noklam! 👍
node_imports = """import logging # noqa | ||
from logging import config # noqa | ||
import logging as dummy_logging # noqa | ||
import logging.config # noqa Dummy import""" |
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.
Why are we using logging now?
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.
@SajidAlamQB I have updated this in the PR and it is included in the PR description. In short, this python module need to be importable. The original code use package1
,package2
which are not real package, as a result we need to do many mocking.
I choose logging
for 2 reasons, 1. it's in standard libraries so no extra dependencies (can be any other standard libraries), 2. It has sub-module so I can test for different kind of import statements. You may swap logging
with os
and it will do the same job.
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
Description
Close #3585
Development notes
It seems like a lot of changes but the main change is I use "real" dummy function instead of string, as a result a lot of mocking and function literals are not needed anymore.
inspect
the module to get itinspect
, I need to import it as a module, so the package need to exist, I uselogging
as the dummy library to replace allpackage1
,package2
etc. We can also mock this but in my experience mocking import will mess up withpytest
. This give me the benefit that I don't need to have a lot of"""xxxxxx"""
, instead I just import the real module and retrieve the information frominspect
.Bonus:
_prepare_function_body
is now basically identical toinspect.getsourcefiles
, so there isn't much to test. We can remove the tests if we want, otherwise it can stay in case we change the implementation in the future.Developer Certificate of Origin
We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a
Signed-off-by
line in the commit message. See our wiki for guidance.If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.
Checklist
RELEASE.md
file