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 python script that tests notebooks. #42

Merged
merged 3 commits into from
Nov 7, 2018
Merged

Add python script that tests notebooks. #42

merged 3 commits into from
Nov 7, 2018

Conversation

grebe
Copy link
Contributor

@grebe grebe commented Sep 29, 2018

Address #32.

The test has a list of expected errors. The script runs each notebook and checks that it has the expected number of errors, and that each error contains a given substring.

Address #32.

The test has a list of expected errors. The script runs each notebook
and checks that it has the expected number of errors, and that each
error contains a given substring.
@edwardcwang
Copy link
Collaborator

edwardcwang commented Sep 29, 2018

Nice! 💯

Two thoughts:

  1. It seems to take quite long to run (>6 minutes and it didn't even finish; see below). I think we should add a flag to allow us to only run it for certain notebooks since it does take a while to run.

(e.g. this is what the Python3 language unit tests have - since it takes a while to run the entire test suite, it provides an option to only run parts of the test suite corresponding to the code you are actively working on.)

  1. It seems to fail at module 3.3 for me for whatever reason...
[NbConvertApp] Converting notebook 3.3_higher-order_functions.ipynb to notebook
[NbConvertApp] Executing notebook with kernel: scala
[NbConvertApp] Writing 26512 bytes to /tmp/tmps3sx34x5.ipynb
3
5
Traceback (most recent call last):
  File "./runtest.py", line 69, in <module>
    check_errors(expected, errors)
  File "./runtest.py", line 35, in check_errors
    assert len(expected) == len(actual)
AssertionError

@grebe
Copy link
Contributor Author

grebe commented Sep 29, 2018

Re: 1, is there a canonical examples for this?

Re: 2, the test might be broken. I was getting timeouts on my current machine so I just pushed hoping that I had gotten it right earlier. It prints out the expected and actual number of errors, so the mismatch means I didn't collect all the errors.

Also, not sure if I'm going crazy, but I think I remembered seeing the number of errors vary from run to run. Keep an eye out for that.

@edwardcwang
Copy link
Collaborator

  1. Just pushed a commit that implements something like that

  2. It seems to be slow and failing on my machine (just ran it again):

[NbConvertApp] Converting notebook 3.3_higher-order_functions.ipynb to notebook
[NbConvertApp] Executing notebook with kernel: scala
[NbConvertApp] Writing 26512 bytes to /tmp/tmpywwuu19s.ipynb
3
5
Traceback (most recent call last):
  File "./runtest.py", line 82, in <module>
    check_errors(expected, errors)
  File "./runtest.py", line 38, in check_errors
    assert len(expected) == len(actual)
AssertionError

@grebe
Copy link
Contributor Author

grebe commented Sep 29, 2018

Working on fixes for the actual tests, it's just slow to run.

Copy link
Contributor

@chick chick left a comment

Choose a reason for hiding this comment

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

This looks good to me.
I do get the following warnings when I run the tests
[NbConvertApp] WARNING | Timeout waiting for IOPub output
Should this be fixed before merging.

@grebe
Copy link
Contributor Author

grebe commented Oct 31, 2018

I think there is a way to change the timeout- not sure what it should be. I think we should add travis (or circleci- are we using that now?) and set the timeout so that whatever we use for CI is happy. I think that we should merge this an fix that up in a later PR adding CI support. If there are no objections, I'll merge.

@grebe grebe merged commit cd0b738 into master Nov 7, 2018
@grebe grebe deleted the addTesting branch November 7, 2018 18:53
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.

3 participants