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

Clarify what is linting and what is testing #109

Closed
ceball opened this issue May 15, 2020 · 5 comments · Fixed by #124 or #127
Closed

Clarify what is linting and what is testing #109

ceball opened this issue May 15, 2020 · 5 comments · Fixed by #124 or #127
Milestone

Comments

@ceball
Copy link
Collaborator

ceball commented May 15, 2020

I'd like to remove the duplication of concepts and code between linting and testing, making nbcelltests simpler and easier to understand for users, and simplifying the code.

nbcelltests.lint allows specification of rules:

max_cells_per_notebook
max_class_definitions
max_function_definitions
max_lines_per_cell
magics_whitelist/magics_blacklist
kernelspec_requirements

lint.run() checks these rules and returns the results (details, plus pass/fail). At the command line, nbcelltests.lint prints the details and exits with status according to pass/fail.

Meanwhile, nbcelltests.test works by generating a unittest class with test_cell methods corresponding to code cells. However, after those methods, the following methods are also added to the class, based on rules supplied to test.run():

test_cell_coverage
test_cells_per_notebook
test_class_definition_count
test_function_definition_count
test_lines_per_cell

Such methods each contain just something like this: assert 5 <= 4 (e.g. where you have said max cells = 4, but there are actually 5 cells).

I think we should remove these rules from nbcelltests.test, and make a clear distinction between what is linting, what is testing, and what is coverage.

Testing refers to cell-by-cell testing (based on execution of) the code. Coverage refers to how many cells have tests*. Linting refers to static checks. I.e. the distinction is not based on how important each type of thing is. Users can easily weight the importance of each as they wish (e.g. if some lint rule is critical to you, you can make sure it fails your process by using the details returned from lint()).

(*) at least for now - we might also have an alternative measure in the future, measuring how much of the notebook code is actually covered

@vidartf
Copy link
Collaborator

vidartf commented May 15, 2020

I'm +1 on removing the "quality tests", and keeping them only as lints. As long as we make the linting part easy to understand that should cover it.

Re the coverage test, I say lets leave that in until we have the improved coverage solution in place. Hopefully this code can help us there: https://github.com/computationalmodelling/nbval/blob/master/nbval/_cover5.py

@ceball
Copy link
Collaborator Author

ceball commented May 19, 2020

Re the coverage test, I say lets leave that in until we have the improved coverage solution in place.

Maybe we should rename the existing "coverage" while we still have chance? E.g. maybe something more like "cell coverage" than "coverage".

@vidartf
Copy link
Collaborator

vidartf commented May 19, 2020

I would say "cell coverage" is a stop-gap until we have line coverage, and should be removed in the future.

@ceball
Copy link
Collaborator Author

ceball commented May 20, 2020

I just realized it's already called cell_coverage anyway :)

@ceball
Copy link
Collaborator Author

ceball commented May 20, 2020

Was closed by #124, github

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