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

Celltest always generated for non-empty code cell, and always runs; defaults to %cell if test is empty #172

Merged

Conversation

ceball
Copy link
Collaborator

@ceball ceball commented Jun 10, 2020

Fixes #169, i.e. implements what is described in the opening comment there.

The important changes are in nbcelltets.tests_vendored - the rest is test updates.

Questions:

  • Decide what magic comment to use for saying "cell is mocked" (i.e. do not run this cell). Currently # no %cell. Vidar proposed # nbcelltest-skip. Or even possibly go straight to Allow notebook authors to control cell skipping/mocking other than by comments in code? #171? (I'd rather try the current underlying behavior out for a bit, then if we are keeping it, use a better mechanism than comment.)
  • Agree it's ok to generate and run a test_cell_3 method (as opposed to not generating/skipping) when no explicit test has been supplied for cell 3, and hence the test defaults to %cell. (This is "case 3 from Proposal for when cells/tests should execute #169.) I think we need some people to try it out in lab etc and give feedback one way or the other.
  • In this PR, a code cell that has an empty ast cannot have a test (exception if it does have a test). Does that seem ok? E.g. would it be reasonable to write # note: there is a test at this cell! in your notebook, and have test code?

To do:

  • Validate implementation matches Proposal for when cells/tests should execute #169
  • Validate it seems to makes sense when using in lab (at least to author!)
  • Upstream equivalent nbval fix for bad magics (not how it's done here) filed an issue/started a PR/it's not related to this PR

Also:

@ceball ceball requested a review from vidartf June 10, 2020 13:04
nbcelltests/tests_vendored.py Outdated Show resolved Hide resolved
@ceball
Copy link
Collaborator Author

ceball commented Jun 10, 2020

@vidartf I have requested a review even though there's a not-yet-done to-do item (that just relates to nbval's handling of bad magics). Meanwhile the other changes and questions are ready for review.

@ceball ceball changed the title WIP: celltest always generated for non-empty code cell, and always runs; defaults to %cell if test is empty Celltest always generated for non-empty code cell, and always runs; defaults to %cell if test is empty Jun 10, 2020
@ceball ceball marked this pull request as ready for review June 12, 2020 11:30
@ceball
Copy link
Collaborator Author

ceball commented Jun 12, 2020

@vidartf I have made this not be WIP - I have removed the nbval change.

Copy link
Collaborator

@vidartf vidartf left a comment

Choose a reason for hiding this comment

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

I didn't fine-comb it, but it LGTM!

@ceball
Copy link
Collaborator Author

ceball commented Jun 12, 2020

We might revisit some things here, but I agree we should merge.

@ceball ceball merged commit a5cd54e into jpmorganchase:master Jun 12, 2020
@ceball ceball added this to the 0.2.1 milestone Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants