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.

Sign up for GitHub

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

Jump to bottom

e2e-test for %load_node magic #3528

Merged
merged 15 commits into from
Jul 10, 2024
Merged

e2e-test for %load_node magic #3528

merged 15 commits into from
Jul 10, 2024

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Jan 18, 2024

Description

The e2e test was not working due to subtle difference between running notebook programatically and from UI. This draft PR was the original tests that I created.

Development notes

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

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

@noklam
e2e draft
8ff3c0d 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam noklam mentioned this pull request Jan 19, 2024
Closed
5 tasks
@noklam noklam mentioned this pull request Mar 12, 2024
Closed
@merelcht merelcht mentioned this pull request Jul 8, 2024
Closed
9 tasks
@noklam
Copy link
Contributor Author

noklam commented Jul 8, 2024

e2e test for Notebook is tricky because we don't have a way to execute notebook as Notebook. This may be due to some front-end logic happening in Jupyter side.

nbconvert provides a function to execute notebook, but the result is different from actually running a cell in notebook.

I think it's still valuable to keep an e2e-test, it would be easier to do this with ipython as it is support natively. We won't be able to test everything (databricks, notebook, ipython, vscode notebook), but they share mostly the same logic so at least we can ensure the parsing bit is correct.

noklam added 7 commits July 9, 2024 15:01
@noklam
Merge branch 'main' into noklam/load-node-e2e
9e11536 
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
@noklam
Merge branch 'noklam/load-node-e2e' of github.com:kedro-org/kedro int…
eeed38a 
…o noklam/load-node-e2e

Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
@noklam
Fix name collision
a7c52cf 
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
@noklam
Add e2e test
bcd9a77 
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
@noklam
protect script under __name__
1674e4c 
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
@noklam
Fix test
6abb874 
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
@noklam
remove test notebook
be53f77 
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
noklam
noklam commented Jul 9, 2024
View reviewed changes
...utter.repo_name }}/src/{{ cookiecutter.python_package }}/pipelines/data_engineering/nodes.py
@@ -17,6 +17,7 @@ def split_data(data: pd.DataFrame, example_test_data_ratio: float) -> dict[str,
The data and the parameters will be loaded and provided to your function
automatically when the pipeline is executed and it is time to run this node.
"""
print("load_node executed successfully")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an anchor statement so that the e2e test and ensure %load_node is run properly without asserting the function definition.

noklam
noklam commented Jul 9, 2024
View reviewed changes
kedro/ipython/__init__.py Outdated
Comment on lines 166 to 168
if local_namespace.get("context") and hasattr(
local_namespace["context"], "project_path"
):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an edge case where I run behave and I get error, because there is a name collision where behave using something also call context in the local namespace.

This is a generic problem though rare.

noklam added 2 commits July 9, 2024 17:34
@noklam
remove debug statement
3bfaeb3 
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
@noklam
revert changes
77d59be 
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
@noklam noklam marked this pull request as ready for review July 9, 2024 16:35
@noklam noklam requested a review from merelcht as a code owner July 9, 2024 16:35
noklam added 2 commits July 9, 2024 17:47
@noklam
fix test
70ae03f 
Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
@noklam
Merge branch 'noklam/load-node-e2e' of github.com:kedro-org/kedro int…
4c475b8 
…o noklam/load-node-e2e

Signed-off-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
merelcht
merelcht reviewed Jul 10, 2024
View reviewed changes
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

Left some small comments, but generally this looks like a good approach. Automatically testing jupyter notebooks has always been a bit weird, so I think it's fine to go with ipython instead.

features/load_node.feature Outdated Show resolved Hide resolved
features/steps/test_starter/{{ cookiecutter.repo_name }}/ipython_script.py Outdated Show resolved Hide resolved
features/steps/util.py Outdated Show resolved Hide resolved
noklam added 2 commits July 10, 2024 11:34
@noklam
clean up notebook e2etest
08632b2 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam
fix test after node rename
262d01b 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam noklam requested review from merelcht and ankatiyar July 10, 2024 12:58
@noklam noklam changed the title Draft e2e-test for %load_node magic e2e-test for %load_node magic Jul 10, 2024
@noklam noklam enabled auto-merge (squash) July 10, 2024 13:07
ankatiyar
ankatiyar approved these changes Jul 10, 2024
View reviewed changes
Copy link
Contributor

@ankatiyar ankatiyar left a comment

Choose a reason for hiding this comment

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

Lgtm!

merelcht
merelcht approved these changes Jul 10, 2024
View reviewed changes
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

Thanks @noklam ! ⭐

@noklam noklam merged commit 12f0986 into main Jul 10, 2024
34 checks passed
@noklam noklam deleted the noklam/load-node-e2e branch July 10, 2024 14:13
@noklam noklam self-assigned this Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ankatiyar ankatiyar ankatiyar approved these changes

@merelcht merelcht merelcht approved these changes

Assignees

@noklam noklam

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@noklam @merelcht @ankatiyar