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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clean up examples and add puppeteer tests #6340

Merged
merged 19 commits into from May 16, 2019
Merged

Clean up examples and add puppeteer tests #6340

merged 19 commits into from May 16, 2019

Conversation

blink1073
Copy link
Member

@blink1073 blink1073 commented May 11, 2019 •
edited

References

Fixes #5819

Code changes

Update example tsconfig and webpack. Cleans up all of the examples to be browser-testable and adds test for all 10 examples.

User-facing changes

All of the examples now build and run without error 馃槃

Backwards-incompatible changes

NA

@blink1073 blink1073 added this to the 1.0 milestone May 11, 2019
@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

ian-r-rose
ian-r-rose approved these changes May 11, 2019
View reviewed changes
Copy link
Member

@ian-r-rose ian-r-rose left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @blink1073, just one optional nit.

scripts/travis_script.sh Outdated
sleep 5
kill $TASK_PID
wait $TASK_PID

Copy link
Member

Choose a reason for hiding this comment

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

Should we popd here? Doesn't matter much right now since it's at the end, but could in the future.

@ian-r-rose
Copy link
Member

ian-r-rose commented May 11, 2019 •
edited

I took a pass at the other services tests and ran into a couple of issues:

  1. The browser example currently fails because tsc is missing from its dependencies in the package.json. It might pass locally if the user has a reasonably up-to-date tsc installed globally.
  2. I couldn't get the node example working because of an _xsrf error. I'm not sure why this is happening, since the token is an empty string in this example. Possibly related to d30ac05?

@blink1073
Copy link
Member Author

Thanks for the testing! I'll dig into those other ones as well.

@blink1073
Copy link
Member Author

@ian-r-rose, the xrsf issue arose if you ran that script from a different directory, now fixed.

I went ahead and added automated testing for all 10 examples 馃槃

@blink1073 blink1073 changed the title Clean up output example and add to tests Clean up output example and add automated tests for all examples May 14, 2019
@blink1073 blink1073 changed the title Clean up output example and add automated tests for all examples [wip] Clean up output example and add to tests May 14, 2019
@blink1073 blink1073 changed the title [wip] Clean up output example and add to tests Clean up examples and add puppeteer tests May 14, 2019
@blink1073 blink1073 closed this May 14, 2019
@blink1073 blink1073 reopened this May 14, 2019
vidartf
vidartf approved these changes May 15, 2019
View reviewed changes
Copy link
Member

@vidartf vidartf left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for cleaning up! I just wanted to highlight that some docstrings in developer helper scripts would be nice (as noted in line comments below). I was mainly thinking along the lines "this is a helper script, it is meant to be called <by X> <when Y> <with Z arguments>, and this is mainly what it does". Anything to help someone who opens the file "out of context" can get a feel for which other files are relevant for the control flow in question :)

examples/example_check.py Show resolved Hide resolved
examples/test_examples.py Show resolved Hide resolved
@blink1073
Copy link
Member Author

Good call, docstrings added.

@vidartf
Copy link
Member

vidartf commented May 16, 2019

LGTM!

@vidartf vidartf merged commit 88cbc53 into jupyterlab:master May 16, 2019
SimonBiggs
SimonBiggs reviewed May 16, 2019
View reviewed changes
packages/services/examples/typescript-browser-with-output/src/index.ts
outputArea.future = kernel.requestExecute({ code });
document.getElementById('outputarea').appendChild(outputArea.node);
await outputArea.future.done;
console.log('Test complete!');
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this @blink1073 :). Really helps me jump back into programming with @jupyterlab tools with the new changes :).

Thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem @SimonBiggs, nice to e-see you again!

Copy link
Member

Choose a reason for hiding this comment

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

:). Also, this does remind me, I do have quite a bit else to thank you for. I learnt so much off of all the work you've done here. For example I have used what I learnt about Lerna here to implement a Python monorepo https://github.com/pymedphys/pymedphys/tree/master/packages. I couldn't have done it without your example, so thank you :).

I also got the ideas of dependency graphing off of you guys:

https://pymedphys.com/developer/dependencies.html

And many, many other things. So, thank you. :) Thanks @blink1073 :).

Copy link
Member Author

Choose a reason for hiding this comment

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

My pleasure. One day we'll meet and you can buy me a beer. ;)

Copy link
Member

Choose a reason for hiding this comment

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

Now that, is a good plan :)

@blink1073 blink1073 deleted the fix-output-example branch June 2, 2019 09:46
@lock
Copy link

lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
@jasongrout jasongrout added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@SimonBiggs SimonBiggs SimonBiggs left review comments

@vidartf vidartf vidartf approved these changes

@ian-r-rose ian-r-rose ian-r-rose approved these changes

Assignees
No one assigned
Labels
enhancement pkg:services status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:DevOps tag:Examples
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.

Jupyter Services Example appears broken
5 participants
@blink1073 @ian-r-rose @vidartf @SimonBiggs @jasongrout