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 extra safety measures, better documentation, better logging, and a separate test to avoid invalid notebook states #153

Merged
merged 9 commits into from
Jul 13, 2022

Conversation

falexwolf
Copy link
Member

@falexwolf falexwolf commented Jul 13, 2022

Add extra safety with a SystemExit and much clearer logging, stratified by interactive vs. non-interactive.

Jupyter Notebook:
image

VS Code:
image

Jupyter Lab:
image

@github-actions
Copy link

github-actions bot commented Jul 13, 2022

@codecov
Copy link

codecov bot commented Jul 13, 2022

Codecov Report

Merging #153 (acb7f7c) into main (ab4dadc) will decrease coverage by 2.73%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##             main     #153      +/-   ##
==========================================
- Coverage   79.75%   77.01%   -2.74%     
==========================================
  Files          17       17              
  Lines         800      805       +5     
==========================================
- Hits          638      620      -18     
- Misses        162      185      +23     
Impacted Files Coverage Δ
nbproject/dev/_meta_store.py 88.23% <50.00%> (ø)
nbproject/_header.py 88.88% <75.00%> (-0.40%) ⬇️
nbproject/dev/_jupyter_lab_commands.py 33.33% <100.00%> (ø)
nbproject/_nbproject_cli.py 62.50% <0.00%> (-15.18%) ⬇️
nbproject/dev/_dependency.py 79.16% <0.00%> (-4.17%) ⬇️
nbproject/_schemas.py 95.55% <0.00%> (-1.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 439fc00...acb7f7c. Read the comment docs.

@falexwolf falexwolf changed the title 🚚 Better name for Jupyter Lab restart function 🚸 Add extra safety measures, better documentation, better logging, and a separate test to avoid invalid notebook states Jul 13, 2022
@falexwolf
Copy link
Member Author

Coverage result without the test:
image

@falexwolf
Copy link
Member Author

Ohhhhhhhh. I can't trigger the SystemExit, because the CLI uses the guides directory as an nbproject folder... That shouldn't be the case! This is what the example project is for. 😅

We need a controlled test environment in the guides directory.

@falexwolf
Copy link
Member Author

falexwolf commented Jul 13, 2022

Without interference of different parts of the testing setup.

Hence, this commit: f113c7a

@Koncopd, is this OK from your perspective? Or does this interfere with any of the rationales you though about when writing the CLI tests? Is it good enough to use example-project as a test for the CLI?

@github-actions github-actions bot temporarily deployed to pull request July 13, 2022 05:30 Inactive
@falexwolf
Copy link
Member Author

falexwolf commented Jul 13, 2022

Now the tests are passing, and trigger-system-exit-upon-init now covers what had previously been covered by the initialize.ipynb notebook.

However, due to no longer running the CLI on all guides, we lose coverage in other parts of the code base:

image

@falexwolf
Copy link
Member Author

We need to restore these parts of the code base by constructing a clean isolated test case for the CLI in the example-project and potentially another folder, if needed.

@Koncopd, happy to discuss this!

@falexwolf falexwolf merged commit 996c3db into main Jul 13, 2022
@falexwolf falexwolf deleted the invalid branch July 13, 2022 05:35
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