-
Notifications
You must be signed in to change notification settings - Fork 73
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
Split off SuiteRunner class #433
base: main
Are you sure you want to change the base?
Conversation
There is no visible perf change, which is expected since this only moves methods to a separate class. Before:
After:
|
Looks like there's a test failure, FYI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good to me, except that loadFrame
should IMO be moved as well in this PR. Tell me what you think!
(and of course like Brian mentioned, the tests are red for some reason :D)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the change, looks good to me
@@ -112,9 +112,9 @@ describe("BenchmarkRunner", () => { | |||
let _runSuiteStub, _finalizeStub, _loadFrameStub, _appendFrameStub, _removeFrameStub; | |||
|
|||
before(async () => { | |||
_runSuiteStub = stub(runner, "_runSuite").callsFake(async () => null); | |||
_runSuiteStub = stub(SuiteRunner.prototype, "_runSuite").callsFake(async () => null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks so brittle :D but let's not fix that now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol... yeah, generally not too happy about how many internals we test, but then again I don't have a clear idea on how to fix this.
Move Suite-related code to a separate SuiteRunner class.
This happens in preparation for async and remote suites where it will be cleaner to handle the code paths independently instead of adding more functionality to the BenchmarkRunner class.