Refactor `TestRunnerService` to only have one instance of `TestRunnerService` that holds references to all test runners

fastlane / ci

Open source, self hosted, mobile optimized CI powered by fastlane

https://fastlane.tools

MIT License

2.08k stars 89 forks source link

Refactor `TestRunnerService` to only have one instance of `TestRunnerService` that holds references to all test runners #216

Closed KrauseFx closed 6 years ago

KrauseFx commented 6 years ago

As seen in https://github.com/fastlane/ci/pull/200/files#diff-62e64a60028b7cd0fdf6248bbc3f9bfcR13, we currently have to store references to all active test_runner_services to keep them alive, and to be able to access them when the user wants to see their progress. I added a comment with a proposal on how we can properly architect it

We probably want a single instance of TestRunnerService (like we do for other services), that holds references to all the active TestRunners

minuscorp commented 6 years ago

Yep, I'm totally with you on this. A single RunnerService must live throughout the application, which holds references to each Runner that might be running at a given moment. One service, many runners.

taquitos commented 6 years ago

@KrauseFx what are the responsibilities that the new refactored TestRunneService will have?

KrauseFx commented 6 years ago

@taquitos

So I was thinking of doing the following

TestRunnerService

Responsible for

Manage all TestRunner objects that are active (either in queue or actually running)
Measures the time of a TestRunner's execution
Stores the Build information in version control and triggers the report of the build status on GitHub

TestRunner

(e.g. FastlaneTestRunner) Responsible for

Loading up fastlane and running a lane with it, checking the return status
Raise an exception if build fails, with information that can be handled by TestRunnerService
Reporting back a list of artifacts to TestRunnerService

Thoughts?

taquitos commented 6 years ago

I love it. One question, and it's a minor one, should we rename them to BuildRunnerService and BuildRunner?

KrauseFx commented 6 years ago

I don't feel strongly about the name BuildRunnerService, so that works. Any specific reason why it's not a service? I thought it behaves similarly to the other fastlane.ci services we already have, as in it stays alive and manages things (state :trollface:)

taquitos commented 6 years ago

I was just saying TestRunnerService -> BuildRunnerService and also TestRunner -> BuildRunner to align us around the build terminology instead of test and job.

KrauseFx commented 6 years ago

Ah right, sorry misunderstood your message, yeah that works 👍

KrauseFx commented 6 years ago

Will work on that refactor today