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
Lab Extension Testing #872
Conversation
This is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still want to look at this more, but here are some initial comments.
check_call(cmd.split(), shell=shell, cwd=cwd) | ||
|
||
|
||
class TestInstallNBExtension(TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestInstallLabExtension
entryPath: './mockextension/index.js', | ||
config: { | ||
output: { | ||
path: path.join(process.cwd(), 'mockextension', 'build'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine that lots of people will be giving explicit output paths for the built files like this (I did with ipywidgets, for example). I think we should add it to the buildExtension configuration, rather than depending on people to always override the webpack config. Perhaps as an outputPath
entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, cf jupyter/extension-builder#8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All review comments addressed as comments or in 7d89515
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in jupyter/extension-builder#9
def test_create_labextensions_system(self): | ||
with TemporaryDirectory() as td: | ||
self.system_labext = pjoin(td, u'labextensions') | ||
with patch.object(labextensions, 'SYSTEM_JUPYTER_PATH', [td]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're patching this out in the setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not this particular directory, I tried removing it and it claimed it did not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular directory is patched in the setup: https://github.com/jupyter/jupyterlab/pull/872/files#diff-d0969d946b6c5e183f26f7cc288e6d04R123
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what you mean here.
with patch.object(labextensions, 'SYSTEM_JUPYTER_PATH', [td]): | ||
install_labextension(self.src, self.name, user=False) | ||
self.assert_installed( | ||
pjoin(self.name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pjoin(self.name)
is equivalent to self.name
. This happens other places too.
) | ||
|
||
def test_create_labextensions_user(self): | ||
with TemporaryDirectory() as td: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why you need a TemporaryDirectory() call in this function.
|
||
config_dir = os.path.join(_get_config_dir(user=True), 'labconfig') | ||
cm = BaseJSONConfigManager(config_dir=config_dir) | ||
enabled = cm.get('jupytlab_config').get('LabApp', {}).get('labextensions', {}).get(self.name, False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jupytlab
-> jupyterlab
?
Thanks! This is huge. |
I read through it again. Looks good to me; thanks! |
cf #728.
Adds local testing for the lab extension mechanism and tests on Travis.