reuse processes using a process pool #195

fabiosantoscode · 2018-10-15T16:29:18Z

No description provided.

1999 · 2018-10-15T20:05:16Z

Wow, seems like a huge work has been made here! 😮

fabiosantoscode · 2018-10-18T16:10:27Z

I've created my own, much simpler package, that replaces mocha-parallel-tests and is able to reuse child processes: https://www.npmjs.com/package/mochallel

1999

@fabiosantoscode I assume you didn't run tests for your changes, did you? Also, it looks like there's some debugging code in your PR. Could you please fix it and update the branch?

mocha.reporter(Reporter).run((code) => {
    onComplete(code);
    throw new Error('foo!');

1999 · 2018-10-31T00:20:23Z

@fabiosantoscode closing this PR as non-active. If you plan to update it feel free to re-open and ping me :)

fabiosantoscode · 2018-10-31T14:02:07Z

Sorry. Can you reopen? I've just pushed a removal of that throw statement.

fabiosantoscode · 2018-10-31T14:02:38Z

I've released an alternative to this module that already does process reusing. It's quite fast! Can you match its speed? http://npmjs.com/mochallel

fabiosantoscode · 2018-10-31T15:17:59Z

@1999

1999 · 2018-10-31T19:22:31Z

@fabiosantoscode I noticed your message above. It's great that it's so fast 👍 Can it be used in mocha-parallel-tests somehow or what do you mean by that?

Also it looks like the build is red, so you also need to fix the errors before we can merge this PR.

fabiosantoscode · 2018-11-02T16:21:26Z

You can use the multiprocess-map module that I created to use in mochallel, if you like

fabiosantoscode · 2018-11-02T16:30:52Z

I meant it as a challenge :) if you can make a faster mochallel, then everybody wins!

fabiosantoscode · 2018-11-02T16:43:01Z

The builds are timing out -- Do you think you know what might be causing it?

fabiosantoscode · 2018-11-02T19:15:03Z

Ok, I think I fixed it.

1999 · 2018-11-02T19:48:05Z

You're almost there: 1 test is failing for node@11.

1999 · 2018-11-02T19:49:11Z

src/main/runner.ts

@@ -149,6 +149,7 @@ export default class RunnerMain extends Runner {
  private emitSubprocessEvents() {
    for (const { event, data, type } of this.subprocessTestResults.events) {
      if (type === 'runner') {
+        assert(event);


This looks like a debug code

It's just to ensure event exists. It might become undefined if a bug is introduced elsewhere.

Looks like a premature optimization. Let's either add a test for it or remove this.

1999 · 2018-11-02T19:49:49Z

src/subprocess/runner.ts


-mocha.reporter(Reporter).run(onComplete);
+  mocha.reporter(Reporter).run((code) => {


You can just write mocha.reporter(Reporter).run(onComplete) I guess?

1999 · 2018-11-02T20:26:37Z

Nice job! Can you think of any tests that we might add here and which might be useful for this PR?

fabiosantoscode · 2018-11-02T20:27:04Z

No, I don't think so.

1999 · 2018-11-02T20:30:04Z

Why not? This PR is a huge improvement to the way mocha-parallel-tests works. For instance if one of the tests is hanging this will stop others from re-using the same process (see #155 for more info). I would like to see all changes covered with tests and here we could for instance check that the process.pid is the same across different files.

fabiosantoscode · 2018-11-03T12:55:51Z

OK, let's do that test.

1999

I looked through the code and it looks like a good first step to process re-using. However, there are at least two pieces not covered with tests and which require your attention. Hopefully, my comments will be useful to you.

1999 · 2018-11-03T19:59:19Z

test/reusing-processes/README

@@ -0,0 +1 @@
+Support mocha --delay option


It looks like you didn't change the copied README file contents

1999 · 2018-11-03T20:02:39Z

test/reusing-processes/index.js

+
+    const [pid1, pid2] = stdout.split(/\n/g).filter(line => /^pid/.test(line));
+
+    if (pid1 && pid2) assert.strictEqual(pid1, pid2);


This looks a bit fragile: if one of the tests doesn't output anything this test will still pass. Do we actually need this check for pid1 and pid2 existence? Isn't it safe to just remove this condition check and run assertion check instead?

Sometimes they don't both come out...

Well, this means that a test is flaky and it should be investigated why it's so. Let's investigate that or remove the test altogether.

I'd still posit that the test has value. It works 90% of the time and could catch a bug in that 90%.

I have a very strong opinion on that. Saying that in 10% of tests one of pid1 or pid2 are empty means that the test hasn't been executed at all because this means that the main process has finished its execution but one of the files which were supposed to get executed didn't do it. This looks like a major bug and I'd spend more time into that. You can skip that, but I will do that later on.

Oh btw I think I know why this can be flaky thing: standard streams are considered to be asynchronous (i.e. console.log is using process.stdout.write which is asynchronous) and this is why listening to their output can be tricky. What you can do is to use a synchronous function to write the process ID for instance fs.writeFileSync. This would guarantee that your PID is saved before the test file has finished executing.

1999 · 2018-11-03T20:11:42Z

test/reusing-processes/index.js

+    const [pid1, pid2] = stdout.split(/\n/g).filter(line => /^pid/.test(line));
+
+    if (pid1 && pid2) assert.strictEqual(pid1, pid2);
+});


Thanks for adding the tests. mocha-parallel-tests is highly covered with tests itself and that's why it's so stable. This PR, however, introduces a very important piece of logic and I'm thinking about at least two additional tests that would make sense here:

Let's assume that we have asynchronous test suite A which is executing for 3 seconds, asynchronous test suite B which is executing for 2 seconds and synchronous test case C; --max-parallel is equal to 2. For this configuration I expect test case C to re-use the process from test case B, while A will be still running in its own process.

The test case from Investigate re-using forked processes #155: what should be the behaviour when the forked process is blocked from exiting (or running the next suite?) by some code (for instance it can launch the http server in "before" hook). I assume we will need to forget about this process and fork new ones but maybe you have a different idea? The reason why we need this is because mocha-parallel-tests is trying to act exactly like mocha. When it can do this it's a shocking surprise for our users.

Could you please also add these tests and maybe you can think of any others?

Ok I'll try to write that test

if some test hoards the process, then I suppose it eventually would time out. I don't think I've implemented that. I'll do it now.

Looks like 2. already times out because we use mocha in the CP as well.

Could you please point me to the test you've written?

What's the CP?

.travis.yml

1999 · 2018-11-03T20:13:25Z

src/main/mocha.ts

@@ -98,6 +100,9 @@ export default class MochaWrapper extends Mocha {

    taskManager
      .on('taskFinished', (testResults: ISubprocessResult) => {
+        if (!testResults) {
+          throw new Error('No output from test');
+        }


I'm not convinced that we need this behaviour:

mocha doesn't have it so if it happens this will be something weird for users

it's not covered with a test

1999 · 2018-11-03T20:28:31Z

src/main/runner.ts

@@ -149,6 +149,7 @@ export default class RunnerMain extends Runner {
  private emitSubprocessEvents() {
    for (const { event, data, type } of this.subprocessTestResults.events) {
      if (type === 'runner') {
+        assert(event);


Looks like a premature optimization. Let's either add a test for it or remove this.

1999 · 2018-11-03T20:31:57Z

src/main/process-pool.ts

+    };
+
+    return process;
+  }


One more thing to consider here is the mocha --exit option. It looks like you call this method right after the end event is got from the subprocess. But in fact, that's not the time when the process is finished: it can have an asynchronous logic which can for instance stop listening for any kind of events and the process can exit with a non-zero exit code in it. In this case, although the test didn't have any errors mocha will show it as failed. So please take a closer look at #155 and maybe #169 comments. Maybe the thing we should do is to use process pool only when mocha is launched with an --exit option.

What do we do then?

That's a question to you. It looks to me like we can re-use processes only when --exit option is specified otherwise we break the mocha behaviour.

@fabiosantoscode ^ any ideas? If no, let's enable this behaviour for --exit-run tests and add a test for that.

1999 · 2018-11-03T20:32:19Z

src/subprocess/runner.ts

@@ -217,42 +184,60 @@ class Reporter extends reporters.Base {
      event: 'sync',
    });

+    assert(event);


Looks like a premature optimization. Let's either add a test for it or remove this.

1999 · 2018-11-03T20:33:36Z

test/full-trace/index.spec.js

+        function x() {
+            throw new Error('foo');
+        }
+        x();


Let's not change existing tests, that's a bad practice. Better create a separate test and add it.

The test was breaking for some reason. I figured adding another stack frame wouldn't hurt.

The test was breaking for some reason

It is not failing in master branch. In any case, changing existing test cases is the last effort. Please revert it back.

@fabiosantoscode ^

1999 · 2018-11-03T20:34:02Z

test/.editorconfig

+insert_final_newline = true
+charset = utf-8
+indent_style = space
+indent_size = 4


fabiosantoscode · 2018-11-03T20:47:27Z

Done @1999

1999 · 2018-11-03T20:48:32Z

Done @1999

Sorry, done what?

fabiosantoscode · 2018-11-03T20:52:24Z

Nevermind

fabiosantoscode · 2018-11-09T17:26:40Z

Implemented all the ideas for tests. Any more tests you can think of?

1999 · 2018-11-10T01:35:58Z

test/process-timeouts/index.1.spec.js

@@ -0,0 +1,5 @@
+describe('suite 1', () => {
+    it('hoard the process', (done) => {
+        setTimeout(done, 10000);


mocha-parallel-tests are executed sequentially, so if you need to have some asynchronous test better use 3 seconds timeout.

This big timeout is meant to be bad, and time out the process.

1999 · 2018-11-10T01:36:41Z

test/process-timeouts/index.js

+const { exec } = require('child_process');
+const libExecutable = resolve(__dirname, '../../dist/bin/cli.js');
+
+exec(`${libExecutable} --max-parallel 1 test/reusing-processes-2/index.1.spec.js test/reusing-processes-2/index.2.spec.js`, {


Our common rule is that the test is using only its own test cases. So it would be great to copy these testcases to test/process-timeouts folder.

My mistake, pushing a fix.

1999 · 2018-11-10T01:37:01Z

test/process-timeouts/index.js

+exec(`${libExecutable} --max-parallel 1 test/reusing-processes-2/index.1.spec.js test/reusing-processes-2/index.2.spec.js`, {
+    cwd: resolve(__dirname, '../../'),
+}, err => {
+    assert(err);


This is a bit generic. It would be great to add a second argument saying what kind of error happened.

It always exits with code 1 I think. It would tell us nothing more about the error.

I mean that the error that you will get if this test fails looks like this now:

AssertionError [ERR_ASSERTION]: null == true
at b (repl:1:11)

And what I think is that the test error should ideally be understandable even without debugging the test. So simple change like that would make things much easier to debug:

assert(err, 'CLI was supposed to exit with an error but it didn\'t')

Ok, changed it

1999 · 2018-11-10T01:37:39Z

test/reusing-processes-2/README

@@ -0,0 +1 @@
+Check if processes are being reused


It would be better if README files were more precise, at least not copies of each other.

1999 · 2018-11-10T01:38:40Z

test/reusing-processes/index.js

+
+    const [pid1, pid2] = stdout.split(/\n/g).filter(line => /^pid/.test(line));
+
+    if (pid1 && pid2) assert.strictEqual(pid1, pid2);


Well, this means that a test is flaky and it should be investigated why it's so. Let's investigate that or remove the test altogether.

fabiosantoscode · 2018-11-14T15:29:32Z

I think you can merge now

1999

@fabiosantoscode thanks for fixing some of the issues. However, there are still some unresolved comments from our previous discussion and I added some more. Please take a look at these and fix them, so we can merge your PR.

1999 · 2018-11-15T20:53:02Z

src/main/mocha.ts

      }

-      debugLog('Process spawned. You can run it manually with this command:');
-      debugLog(`node ${nodeFlags.join(' ')} ${runnerPath} ${forkArgs.concat([DEBUG_SUBPROCESS.argument]).join(' ')}`);
+      test.send(JSON.stringify({ type: 'start', testOptions }));


You did but what's the idea of passing a string if your definition says that the msg can be any? I'd prefer hiding this logic inside the "send" function (i.e. do JSON.stringify in case it's needed there).

1999 · 2018-11-15T20:53:18Z

src/main/mocha.ts

+      try {
+        test = await this.pool.getOrCreate(this.isTypescriptRunMode);
+      } catch (e) {
+        throw e;


@fabiosantoscode ^

1999 · 2018-11-15T20:53:25Z

src/main/mocha.ts

        } else {
+          assert(event);


@fabiosantoscode ^

1999 · 2018-11-15T20:56:45Z

src/main/process-pool.ts

+  kill: () => void;
+  destroy: () => void;
+  on: (event: string, fn: eventFn) => void;
+  removeListener: (event: string, eventFn) => void;


These two methods look very similar to node EventEmitter, but they're doing different things. Could you please rename them to be more explicit? For instance "on" -> "addProcessListener" and "removeListener" -> "removeProsessListener".

Another approach would be that IMochaProcess objects could be extending node EventEmitter but in this case, you'd have to re-implement all EventEmitter methods related to adding/removing listeners.

1999 · 2018-11-15T20:58:52Z

src/main/process-pool.ts

+    });
+  }
+
+  create(runnerPath: string, opt: object): IMochaProcess {


opt is too vague. Could you add some interface check for it? As far as I see it should be the ForkOptions from "child_process".

1999 · 2018-11-15T21:02:40Z

src/main/process-pool.ts

+    };
+
+    return process;
+  }


@fabiosantoscode ^ any ideas? If no, let's enable this behaviour for --exit-run tests and add a test for that.

1999 · 2018-11-15T21:03:04Z

test/full-trace/index.spec.js

+        function x() {
+            throw new Error('foo');
+        }
+        x();


@fabiosantoscode ^

1999 · 2018-11-15T21:03:18Z

test/index.sh

@@ -29,6 +30,9 @@ function test {
 MOCHA_VERSION=`mocha --version`
 echo "You're running tests with mocha version $MOCHA_VERSION"

+test 'process reuse' test/reusing-processes/index.js
+test 'process reuse (2)' test/reusing-processes-2/index.js


Let's have a more explicit name for this test

1999 · 2018-11-15T21:06:19Z

test/reusing-processes/index.js

+
+    const [pid1, pid2] = stdout.split(/\n/g).filter(line => /^pid/.test(line));
+
+    if (pid1 && pid2) assert.strictEqual(pid1, pid2);


I have a very strong opinion on that. Saying that in 10% of tests one of pid1 or pid2 are empty means that the test hasn't been executed at all because this means that the main process has finished its execution but one of the files which were supposed to get executed didn't do it. This looks like a major bug and I'd spend more time into that. You can skip that, but I will do that later on.

1999 · 2018-11-15T21:07:54Z

test/reusing-processes/index.js

+    const [pid1, pid2] = stdout.split(/\n/g).filter(line => /^pid/.test(line));
+
+    if (pid1 && pid2) assert.strictEqual(pid1, pid2);
+});


Could you please point me to the test you've written?

What's the CP?

1999 requested changes Oct 21, 2018

View reviewed changes

1999 closed this Oct 31, 2018

1999 reopened this Oct 31, 2018

fabiosantoscode force-pushed the master branch from e0e3afb to 963a977 Compare November 2, 2018 16:42

1999 reviewed Nov 2, 2018

View reviewed changes

fabiosantoscode force-pushed the master branch from 48e4ac9 to ae6eeba Compare November 2, 2018 19:50

fabiosantoscode force-pushed the master branch 4 times, most recently from 9d613fd to 60abaa5 Compare November 3, 2018 18:21

1999 requested changes Nov 3, 2018

View reviewed changes

fabiosantoscode force-pushed the master branch from 60abaa5 to 5f73f22 Compare November 3, 2018 20:54

fabiosantoscode added 2 commits November 3, 2018 21:01

reuse processes using a process pool

d37d513

change style, and complete process wrapper

92921f9

fabiosantoscode force-pushed the master branch from 5f73f22 to 92921f9 Compare November 9, 2018 16:48

fabiosantoscode added 2 commits November 9, 2018 16:54

GCC is already 4.8

756d770

test process reuse and timeouts

5048a63

1999 requested changes Nov 10, 2018

View reviewed changes

fabiosantoscode added 3 commits November 10, 2018 17:07

use own test in process-timeouts

85477fe

further test documentation

833d66c

add assertion message

0777db2

1999 requested changes Nov 15, 2018

View reviewed changes


		mocha.reporter(Reporter).run(onComplete);
		mocha.reporter(Reporter).run((code) => {


		const [pid1, pid2] = stdout.split(/\n/g).filter(line => /^pid/.test(line));

		if (pid1 && pid2) assert.strictEqual(pid1, pid2);

reuse processes using a process pool #195

Are you sure you want to change the base?

reuse processes using a process pool #195

Conversation

fabiosantoscode commented Oct 15, 2018

1999 commented Oct 15, 2018

fabiosantoscode commented Oct 18, 2018

1999 left a comment

Choose a reason for hiding this comment

1999 commented Oct 31, 2018

fabiosantoscode commented Oct 31, 2018

fabiosantoscode commented Oct 31, 2018

fabiosantoscode commented Oct 31, 2018

1999 commented Oct 31, 2018 • edited Loading

fabiosantoscode commented Nov 2, 2018 • edited Loading

fabiosantoscode commented Nov 2, 2018

fabiosantoscode commented Nov 2, 2018

fabiosantoscode commented Nov 2, 2018

1999 commented Nov 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1999 commented Nov 2, 2018

fabiosantoscode commented Nov 2, 2018

1999 commented Nov 2, 2018

fabiosantoscode commented Nov 3, 2018

1999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiosantoscode commented Nov 3, 2018

1999 commented Nov 3, 2018

fabiosantoscode commented Nov 3, 2018

fabiosantoscode commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiosantoscode commented Nov 14, 2018

1999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1999 commented Oct 31, 2018 •

edited

Loading

fabiosantoscode commented Nov 2, 2018 •

edited

Loading