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

Update workspace services. #4841

Merged
merged 31 commits into from Jul 24, 2018
Merged

Update workspace services. #4841

merged 31 commits into from Jul 24, 2018

Conversation

@afshin
Copy link
Member

@afshin afshin commented Jul 6, 2018

This PR has a hard dependency on this jupyterlab_launcher PR: jupyterlab/jupyterlab_server#48

  • Add new workspace service methods.
  • Switch workspace and setting services to async/await
  • Write tests for workspace methods.
  • Use LabTestBase from jupyterlab_launcher and simplify tests.
  • Fix tests to support .tsx files.
  • Fix defunct dialog.spec.tsx tests.
  • Update to jupyterlab_launcher>=0.11.2,<0.12.0
@afshin afshin self-assigned this Jul 6, 2018
@afshin afshin added this to the Beta 4 milestone Jul 6, 2018
@afshin afshin force-pushed the workspace-services branch from 0a75bfb to c23bd9b Jul 6, 2018
@afshin afshin changed the title [wip] Add a list() method to the workspace service. [wip] Update workspace services. Jul 6, 2018
const url = Private.url(base, '');
const promise = ServerConnection.makeRequest(url, {}, serverSettings);

return promise
Copy link
Member

@blink1073 blink1073 Jul 6, 2018

Choose a reason for hiding this comment

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

Why no await?

Copy link
Member Author

@afshin afshin Jul 6, 2018

Choose a reason for hiding this comment

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

Ha! It was not yet pushed.

@afshin afshin force-pushed the workspace-services branch from a29ff8a to 0a6efd6 Jul 6, 2018
// Copyright (c) Jupyter Development Team.
// Distributed under the terms of the Modified BSD License.

import expect = require('expect.js');
Copy link
Member

@blink1073 blink1073 Jul 9, 2018

Choose a reason for hiding this comment

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

Are we not using chai for new tests?

Copy link
Member Author

@afshin afshin Jul 9, 2018

Choose a reason for hiding this comment

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

All the tests in services appear to be using expect.js.

Copy link
Member

@blink1073 blink1073 Jul 9, 2018

Choose a reason for hiding this comment

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

I'll bring it up on Wednesday. I think we should use chai moving forward: for new files, and when adding new tests to files, with the eventual goal to use it everywhere.

Copy link
Member Author

@afshin afshin Jul 9, 2018

Choose a reason for hiding this comment

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

Sounds good. For services, I think we should do them all at once in one PR, probably. What do you think?

Copy link
Member

@blink1073 blink1073 Jul 9, 2018

Choose a reason for hiding this comment

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

That's a big matzo ball...

Copy link
Contributor

@jasongrout jasongrout Jul 9, 2018

Choose a reason for hiding this comment

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

I'll bring it up on Wednesday. I think we should use chai moving forward: for new files, and when adding new tests to files, with the eventual goal to use it everywhere.

I've been holding back for consistency, but +1 to moving to chai. I figured we might just do it in one huge pass, but happy to do it incrementally as well.

@afshin
Copy link
Member Author

@afshin afshin commented Jul 9, 2018

The server process that runs in the services tests does not seem to answer /settings and /workspaces requests. I'm assuming it's because it's not running those services out of jupyterlab_launcher but I don't know how to update it to have those services so that their corresponding front-end wrappers can be tested.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 9, 2018

We need to add handlers to the ProcessApp, perhaps in the start method. Something like this pattern.

@afshin
Copy link
Member Author

@afshin afshin commented Jul 10, 2018

Thanks for the tip, @blink1073!

@afshin
Copy link
Member Author

@afshin afshin commented Jul 10, 2018

I am not sure how to pass the test configuration data (from config.json) through to the ProcessApp that now supports all the additional handlers as of this commit: jupyterlab/jupyterlab_server@680bf97

@afshin afshin force-pushed the workspace-services branch 2 times, most recently from 97b1cd6 to d2b4f07 Jul 13, 2018
@afshin afshin force-pushed the workspace-services branch from 9805b2c to 8455349 Jul 17, 2018
@afshin afshin changed the title [wip] Update workspace services. Update workspace services. Jul 17, 2018
@@ -142,14 +141,15 @@ describe('docregistry/context', () => {
await context.initialize(false);
await context.ready;
expect(context.model.cells.canUndo).to.be(false);
await dismissDialog();
Copy link
Member

@ian-r-rose ian-r-rose Jul 18, 2018

Choose a reason for hiding this comment

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

I think that this may not be necessary now that there is an await above (from #4895). The dialog is spurious due to a race condition, no?

Copy link
Member Author

@afshin afshin Jul 18, 2018

Choose a reason for hiding this comment

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

Good call. It's gone now.

@afshin afshin force-pushed the workspace-services branch from fddab70 to 6f60bbe Jul 20, 2018
@afshin afshin force-pushed the workspace-services branch from 31512b4 to 270fe78 Jul 23, 2018
@afshin afshin force-pushed the workspace-services branch from 28b3055 to 91abdfd Jul 24, 2018
@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 24, 2018

Thanks for all the clean up!

@blink1073 blink1073 merged commit 69c96de into jupyterlab:master Jul 24, 2018
2 checks passed
@afshin afshin mentioned this pull request Jul 24, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants