chore(refactor): Pull proxy logic out and add unit tests around it. #22
Conversation
heathkit
force-pushed
the
2-webdriver-refactor
branch
2 times, most recently
from
5c04379 to
8dca8ae
Compare
| @@ -1,6 +1,8 @@ | |||
| /** | |||
| * Utilities for parsing WebDriver commands from HTTP Requests. | |||
| */ | |||
| import * as events from 'events'; | |||
| import * as http from 'http'; | |||
There was a problem hiding this comment.
Yup, I'll turn on the no-unused-variable lint check.
There was a problem hiding this comment.
Or, rather, TypeScript 2.1 now has --noUnusedLocals
| } | ||
| } | ||
|
|
||
| export interface WebDriverBarrier { onCommand(command: WebDriverCommand): Promise<void>; } |
| }, replyWithError); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Class-level comment for WebDriverBarrier would be nice.
| this.barriers.push(barrier); | ||
| } | ||
|
|
||
| requestListener(originalRequest: http.IncomingMessage, response: http.ServerResponse) { |
There was a problem hiding this comment.
What about renaming requestListener to request or handle?
| response.end(); | ||
| }; | ||
|
|
||
| // TODO: What happens when barriers error? return a client error? |
There was a problem hiding this comment.
I think we should reply to the initial response with a 500 and an error message chosen by the barrier.
There was a problem hiding this comment.
Maybe, if the barrier rejects its promise, we use that value as the error.
| }; | ||
|
|
||
| // TODO: What happens when barriers error? return a client error? | ||
| let barrierPromises = this.barriers.map((b) => b.onCommand(command)); |
There was a problem hiding this comment.
This could result in multiple barriers executing at once, which might confuse webdriver if for example two barriers are both calling executeScript. I think we should gate them to running one at a time.
There was a problem hiding this comment.
Good point, fixed.
|
|
||
| it('should provide hooks when relaying commands', | ||
| () => { | ||
|
|
| done(); | ||
| }); | ||
| }); | ||
| }); |
| let proxy: WebDriverProxy; | ||
|
|
||
| beforeEach(() => { | ||
| proxy = new WebDriverProxy(`http://localhost:4444/wd/hub`); |
There was a problem hiding this comment.
This is a nonsense url, right? Can we make that clearer, like http://test_webdriver_url?
heathkit
force-pushed
the
2-webdriver-refactor
branch
from
8dca8ae to
f3f81df
Compare
| * A proxy that understands WebDriver commands. Users can add middleware (similar to middleware in | ||
| * express) that will be called before | ||
| * forwarding the request to WebDriver or forwarding the response to the client. | ||
| * A proxy that understands WebDriver commands. Users can add barriers * (similar to middleware in |
There was a problem hiding this comment.
Extra * made it into this line.
|
LGTM with one typo. |
Note that this is based on #21 and should be reviewed after that's merged.
This pulls out the the proxy/wait behavior into a separate module so we can unit test it. The idea is that the proxy waits for promises from barriers (not married to the name, suggestions welcome) to resolve before proceeding.