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

Use Jest for Services Tests #5251

Merged
merged 3 commits into from Sep 11, 2018
Merged

Use Jest for Services Tests #5251

merged 3 commits into from Sep 11, 2018

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 31, 2018

cf #5173

Notes:

  • We can't pass CLI to node, so updated PageConfig to accept process.env.JUPYTER_CONFIG_DATA as a path
  • Adds a utility to convert a few of the mocha directives to their jest equivalents
  • We still need ws for this suite, because we are using a mock server for some tests
  • Created a JestApp that will eventually be shared by all of the test suites and allows easy flags for coverage, watching changed or all files, and for testing specific patterns.
  • We could also stop using chai and use the jest utilities instead (but not in this PR).
  • We'll want to use --onlyFailures to rerun flaky tests when that is available: facebook/jest#6470

cc @saulshanabrook

@blink1073 blink1073 added this to the 0.35 milestone Aug 31, 2018
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 31, 2018

There are still some tests failing locally, I'll return to this after the holiday.

@blink1073 blink1073 mentioned this pull request Sep 4, 2018
7 tasks
@blink1073 blink1073 changed the title [WIP] Use Jest for Services Tests Use Jest for Services Tests Sep 5, 2018
@blink1073 blink1073 changed the title Use Jest for Services Tests [WIP] Use Jest for Services Tests Sep 5, 2018
@blink1073 blink1073 removed this from the 0.35 milestone Sep 5, 2018
@blink1073 blink1073 added this to the 1.0 milestone Sep 5, 2018
@blink1073 blink1073 changed the title [WIP] Use Jest for Services Tests Use Jest for Services Tests Sep 8, 2018
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 8, 2018

Good to go!

Copy link
Member

@saulshanabrook saulshanabrook left a comment

Would it be possible to hide console.log's from test output? there are lots of them and makes it harder to parse output: https://travis-ci.org/jupyterlab/jupyterlab/jobs/426068176#L625

Also, this seems to actually be slower than the previous karma setup.

Ah I guess you did add some more tests. So that might account for it.

env['JUPYTER_CONFIG_DATA'] = config_path
return cmd, dict(cwd=self.jest_dir, env=env)


class KarmaTestApp(ProcessTestApp):
Copy link
Member

@saulshanabrook saulshanabrook Sep 8, 2018

Can this be removed?

Copy link
Member

@saulshanabrook saulshanabrook Sep 8, 2018

Woops, I forgot to read the title of this issue :) I thought this replaced all tests, not just services tests.

@@ -0,0 +1,17 @@
const path = require('path');
Copy link
Member

@saulshanabrook saulshanabrook Sep 8, 2018

Do we need this file still?

Copy link
Member

@saulshanabrook saulshanabrook Sep 8, 2018

Yes we do, forgot I said this.

defaultKernel.restart();
await emission;
});
// it('should get a restarting status', async () => {
Copy link
Member

@saulshanabrook saulshanabrook Sep 8, 2018

Can we use "skip" here instead of commenting out so we know we are skipping it?

defaultKernel.restart();
await defaultKernel.ready;
});
// it('should restart and resolve with a valid server response', async () => {
Copy link
Member

@saulshanabrook saulshanabrook Sep 8, 2018

Same here

expect(future.isDisposed).to.equal(true);
expect(comm.isDisposed).to.equal(true);
});
// it('should dispose of existing comm and future objects', async () => {
Copy link
Member

@saulshanabrook saulshanabrook Sep 8, 2018

and here

expect((await manager.list()).sort()).to.deep.equal(ids.sort());
});
});
// describe('#list()', async () => {
Copy link
Member

@saulshanabrook saulshanabrook Sep 8, 2018

same

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Sep 8, 2018

Looks good overall! It was less changes than I expected...

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 8, 2018

Cool, thanks! I'll uncomment and skip instead.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 8, 2018

As for slowness, I did not expect this one to be a win per se, since it wasn't using Karma. I did this one first because it was less intrusive and will allow us to be consistent across the tests.

@blink1073 blink1073 changed the title Use Jest for Services Tests [WIP] Use Jest for Services Tests Sep 8, 2018
@blink1073 blink1073 changed the title [WIP] Use Jest for Services Tests Use Jest for Services Tests Sep 8, 2018
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 8, 2018

Skipped the tests, added --silent unless debugging. Another point about the slowness - we are re-executing the startup script for every module. Something to keep in mind, to keep that file as slim as possible.

@blink1073 blink1073 changed the title Use Jest for Services Tests [WIP] Use Jest for Services Tests Sep 8, 2018
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 8, 2018

Put it back to WIP because I want to make sure coverage is actually working.

blink1073 added 2 commits Sep 8, 2018
update deps

wip jest testing

wip jest testing

finish jest updates

integrity

Fix build:test

Cleanup

Add mocha types

Isolate test

Isolate contents test

Use jlpm

build utils

fix script

Try all contents tests

run all tests

revert changes to travis script

Allow services to fail once

Clean up shutdown and fix race conditions

fix terminal race condition

clean up contents test

Skip tests instead of commenting them

Silence when not debugging

Fix coverage script

Clean up coverage handling
@blink1073 blink1073 changed the title [WIP] Use Jest for Services Tests Use Jest for Services Tests Sep 8, 2018
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Sep 8, 2018

Ready again!

afshin
afshin approved these changes Sep 11, 2018
Copy link
Member

@afshin afshin left a comment

🙌
👍

@afshin afshin merged commit f455a36 into jupyterlab:master Sep 11, 2018
2 checks passed
@blink1073 blink1073 removed this from the 1.0 milestone Sep 28, 2018
@blink1073 blink1073 added this to the 0.35 milestone Sep 28, 2018
@blink1073 blink1073 mentioned this pull request Sep 28, 2018
31 tasks
@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

3 participants