-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Conversation
I would pull out the I would probably also store the initial working dir and change back to it at the end, not sure if it makes a difference, but it feels cleaner. I can't think of a good way to test it either |
Yeah, I don't think we can test the thing which runs the tests in the tests 😆 Once we set up CI which runs the pip installation, this will be tested implicitly by that, so I think we're all good. |
I realized that if the Is it worth adding these packages to the requirements installed by pip? Alternatively, we could fix #1200 ... |
The whole reason |
perhaps we can move these essential testing packages (pytest, flaky, responses>=0.7, jupyter) to requirements.txt and the setup.py no need for a user to install sphinx or numpydoc or coverage... |
I think others have stronger / more informed opinions here than me, so I'll let someone else answer the question. I was just providing some context. |
Jupyter is enormous and is currently in our |
another option is to turn off notebook tests by default in |
ANOTHER option is to rewrite the notebook tutorials as markdown tutorials and get rid of our jupyter dependency altogether 😇 |
@nelson-liu Either solution you propose is fine - we can re-write the tutorials in a separate PR |
* Add allennlp test-install command * Add back a newline that I accidentally removed * Add small test for _get_project_root * Fix typo in parens * Remove erroneous copy/paste * Add docs for test-install * Remove some unused imports in commands.__init__.py * Turn off notebook tests by default as well * Raise informative error message if jupyter not found * Move pytest, flaky, responses to requirements
Fixes #856 by creating an
allennlp test-install
command that runs the unit test suite.I've tested it locally outside of my allennlp install directory and it seems to work fine for both options, but unsure how to write a unit test for this.
This command skips the sniff tests by default, which should address @DeNeutoy 's concern from #1208 ?