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

[#4949] WIP: fix: transpile once only #4975

Conversation

joscha
Copy link

@joscha joscha commented Jan 24, 2020

This PR exemplifies potential changes to fix the issue described in #4949.

It is a WIP and is there to discuss how this can be made to work in a better way.
The general gist is:

  • it removes the child process used for the test execution
  • it injects a dependency into all packages that assume to be executed in a child process which use process.send to communicate with the parent process that is used instead for communication.
  • Reporting works as expected in contrary to [#4949] WIP: feat: batch specs per suite #4974

@@ -73,9 +73,14 @@ class CucumberAdapter {
let result

try {
// we need tp ensure that we don't run the registration code
Copy link
Author

Choose a reason for hiding this comment

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

this change can potentially go away if we only ever have one Runner instance that we reuse across runs.

Copy link
Member

@baruchvlz baruchvlz left a comment

Choose a reason for hiding this comment

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

Some code style comments.

packages/wdio-cucumber-framework/src/index.js Show resolved Hide resolved
packages/wdio-local-runner/src/worker.js Show resolved Hide resolved
@joscha
Copy link
Author

joscha commented Jan 24, 2020

Thanks for the review @baruchvlz - did you look at the original issue? Do you have some non-stylistic feedback as well by any chance?

@baruchvlz
Copy link
Member

@joscha I will still want to indent the block, don't want anyone else in the future thinking that this is ok :).

I would a bit more clarity on why you're using the FakeChildProcess class, I read the comment but it feels a bit hacky to me.

@joscha
Copy link
Author

joscha commented Jan 29, 2020

Yes, the FakeChildProcess is a workaround for now. I don't intend to merge this as-is. The pull request is just to discuss whether this approach would be viable in general. I was hoping for comments, discussion and potentially a better approach from the maintainers but it seems to me that this might not happen, so maybe I should clean it up after all and we discuss actual mergeability.

@baruchvlz
Copy link
Member

Ah, I see. Sorry for the misunderstanding you can ignore my comments. I do have a question then, why can't we just transpile once before running and run the tests from those files instead of having to transpile on every execution?

but it seems to me that this might not happen

Maybe the lack of comments is, in itself, a way of saying that this isn't the correct approach. And to be honest I'm not entirely convinced that passing this target to the Runner is the best solution.

it removes the child process used for the test execution

Correct me if I'm wrong, but wouldn't this affect test execution? What will happen to concurrency if you remove child processes and everything runs on the main thread?

but it seems to me that this might not happen, so maybe I should clean it up after all and we discuss actual mergeability

Yea, this would probably be the fastest and best approach. You have to keep in mind that maintainers and committers have other responsibilities and must of us do this on our free time, so for some is hard to jump at every pull request and engage in conversations about it at all times. Completed pull requests will gain the most traction.

@christian-bromann
Copy link
Member

This approach won't work because it eliminates the ability to run tests in parallel. With Mocha (not sure about Jasmine and Cucumber) it just won't work to create multiple runner instances and have them run in parallel.

@joscha
Copy link
Author

joscha commented Apr 6, 2020

Okay will close. Thanks for the feedback!

@joscha joscha closed this Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants