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

Fix code/test correspondence; explicitly skip test empty cells/tests. #112

Merged
merged 5 commits into from
May 19, 2020

Conversation

ceball
Copy link
Collaborator

@ceball ceball commented May 15, 2020

Fixes #97 and fixes #99. Same tests as in #111, just updated for 1-based counting.

Summary of new behavior

  1. Test methods are only generated for code cells. If a non-code cell contains a test in its metadata, it's an error at generation time. Only code cells will be included in "cell coverage" calculation.

  2. Test methods are named like test_code_cell_n (so you know it's only code cells), where n is the 1-based code cell number - i.e. they are numbered like execution count (if run from scratch).

Example: If there are 3 cells, and the 2nd cell is markdown, then there'll just be two test methods, test_code_cell_1 and test_code_cell_2.

  1. Code cells that are empty or do not have a test still generate a test method, but decorated with @skip(reason) (where reason is "empty code cell" or "cell has no test").

  2. Tests that do not contain a cell injection generate a skipped test method (reason is "no cell injection").

Changes in this PR

  • Fix correspondence of code and test cells (e.g. markdown caused them to go out of sync)

  • Change cell/test numbering to start from 1, and only include code cells (matching other notebook tools).

  • If a non-code cell contains test in metadata, it's an error during test script generation.

  • Create explicitly skipped tests for:

    • empty cell
    • empty test
    • code cell not injected into test

To do:

  • Fix windows tests

Future work:

EDIT: clarified that coverage will be addressed in a subsequent PR once behavior here is agreed.

  to go out of sync).

* Change cell/test numbering to start from 1, and only include code
  cells (matching other notebook tools).

* If a non-code cell contains test in metadata, it's an error during
  test script generation.

* Create explicitly skipped tests for:
    * empty cell
    * empty test
    * code cell not injected into test
@ceball ceball mentioned this pull request May 15, 2020
2 tasks
@vidartf
Copy link
Collaborator

vidartf commented May 19, 2020

I see there is still an unchecked todo here. Is this ready for review?

@vidartf
Copy link
Collaborator

vidartf commented May 19, 2020

Also: Does this supersede #111 then?

@ceball
Copy link
Collaborator Author

ceball commented May 19, 2020

#111 just demonstrates the same tests as are here failing on master. #111 contains only tests and is not for merging; it's for information only.

I see there is still an unchecked todo here. Is this ready for review?

@vidartf Yes, it's ready for review. I have removed the to-do. Since the coverage calculation* is not altered here, and is not tested, I should be allowed to address that in a future PR I think - after the behavior of testing here is agreed. The behavior changes here imply that the coverage calculation should change. It's currently:

num code cells with %cell in their tests / num code cells

This PR being accepted implies coverage should change to:

num non-empty code cells with %cell in their tests / num non-empty code cells

Note that the current definition of coverage can be gamed by adding empty code cells with tests :)

(*) Actually there are three separate calculations of coverage in the code. Two of them are the same, one is broken. Still, none of those three calculations or components thereof are touched by this PR.

nbcelltests/test.py Outdated Show resolved Hide resolved
nbcelltests/shared.py Show resolved Hide resolved
nbcelltests/test.py Show resolved Hide resolved
nbcelltests/tests/test_test.py Show resolved Hide resolved
nbcelltests/tests/test_test.py Show resolved Hide resolved
nbcelltests/test.py Show resolved Hide resolved
@vidartf
Copy link
Collaborator

vidartf commented May 19, 2020

This PR being accepted implies coverage should change to: ...

I would say the final goal would be to get proper line/branch coverage reports :)

@ceball
Copy link
Collaborator Author

ceball commented May 19, 2020

This PR being accepted implies coverage should change to: ...

I would say the final goal would be to get proper line/branch coverage reports :)

Ok, I should have said the "cell tests coverage" or similar, to distinguish from actual code coverage (see also #109).

@ceball
Copy link
Collaborator Author

ceball commented May 19, 2020

Thanks, I'll follow up what we discussed here (e.g. creating an issue to track pytest in pytest skip handling) and then merge once I've done that.

@ceball
Copy link
Collaborator Author

ceball commented May 19, 2020

Hmm, azure doesn't seem to be working here any more?

@ceball ceball closed this May 19, 2020
@ceball ceball reopened this May 19, 2020
@ceball ceball linked an issue May 19, 2020 that may be closed by this pull request
@ceball ceball merged commit c99d472 into master May 19, 2020
@ceball ceball deleted the fix_cell_test_counting branch May 20, 2020 06:32
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.

Generated test methods for empty cells are confusing Testing a non-code cell confuses test collection
2 participants