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

Fix channel closed error #219

Merged
merged 8 commits into from
Feb 27, 2017
Merged

Fix channel closed error #219

merged 8 commits into from
Feb 27, 2017

Conversation

nicojs
Copy link
Member

@nicojs nicojs commented Jan 16, 2017

Please have a look at the new Task api i've implemented. I think it looks a bit cleaner. Also removed the restart functionality from the IsolatedTestRunnerAdapter and put it in a TimeoutDecorator class. It looks a lot cleaner in my opinion. Its also easier to add debug functionality now, where the TestRunner runs in the same process as Stryker itself, because the timeout logic is not tightly coupled anymore with the spawning of new processes.

New functionality can now be added in TestRunnerDecorator classes. The RetryDecorator implements retries whenever a child process dies (EPIPE error).

* Add an integration test for test runner that crashes with process.exit();
* Group all integration test test runners for maintanability
Replace the api based on lose promises with a `Task` class. Looks more readable.
* Refactor lose promises to use a simple Task-based api.
* Make sure a test runner gets restarted when the child process crashes.
* Introduced a new `TestRunnerDecorator` base class which can be used to decorate a `TestRunner` with additional functionality.
* Introduced a new `RetryDecorator` which is wrapped around the `TimeoutDecorator`.
* Rename `IsolatedTestRunnerAdapterFactory` => `ResilientTestRunnerFacotry`.
* Fix bithound issue, readonly not yet supported.
@nicojs nicojs changed the title WIP: Fix channel closed error Fix channel closed error Jan 19, 2017
@nicojs
Copy link
Member Author

nicojs commented Jan 30, 2017

As said during our hackathon, the Decorator post fix can be confusing, as people might confuse it with TypeScript decorators. However, i want to keep it like this, as we're implementing a decorator design pattern. I've looked into ways of utilizing TypeScript decorators. It will work, but it won't be as easy to read.

I've also thought about renaming IsolatedTestRunnerAdapter to IsolatedProcessDecorator, but the constructor looks different from the TestRunnerDecorator base class, so we can't inherit from that (also: inheritance wouldn't add much, as we can't use any of the default implementations in that class).

@nicojs
Copy link
Member Author

nicojs commented Feb 24, 2017

Ok, all issues fixed. Could you have a look to it again @simondel ?

@@ -1,4 +1,5 @@
import { MutantStatus, MatchedMutant, MutantResult } from 'stryker-api/report';
import * as sinon from 'sinon';
Copy link
Member

Choose a reason for hiding this comment

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

Why this import?

export default class RetryDecorator extends TestRunnerDecorator {
private currentRunTask: Task<RunResult>;

run(options: RunOptions): Promise<RunResult> {
Copy link
Member

Choose a reason for hiding this comment

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

Why not pass the RunOptions in a constructor? That way you have to create a new RetryDecorator and you will not keep any state by accident.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure i follow your logic. The RunOptions here are provided in the run method as specified on the TestRunner interface.

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, I was confused with the Config class.

Copy link
Member Author

Choose a reason for hiding this comment

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

Takes you forever to merge!

@simondel simondel merged commit 202d4b5 into master Feb 27, 2017
@simondel simondel deleted the fix-channel-closed-error branch February 27, 2017 15:54
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.

2 participants