-
Notifications
You must be signed in to change notification settings - Fork 9
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 socket bug introduced in Roku OS 13.0 #132
Conversation
Add `getRtaConfig` to allow getting full config from components Improve test screenshot file naming
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.
I had a few small comment changes (not critical), and then one parameter name suggestion. But otherwise, looks pretty good.
client/src/utils.ts
Outdated
@@ -199,7 +200,7 @@ class Utils { | |||
return error; | |||
} | |||
|
|||
public getTestTitlePath(contextOrSuite: Mocha.Context | Mocha.Suite) { | |||
public getTestTitlePath(contextOrSuite: Mocha.Context | Mocha.Suite, sterilize = true) { |
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.
I feel like sanitize
is a better name for this? sterilize
feels a little off. haha
client/src/ECP.ts
Outdated
@@ -51,11 +51,16 @@ export class ECP { | |||
this.device.setConfig(config); | |||
} | |||
|
|||
public getConfig() { | |||
/** Provides a way to get the whole config not just this classes' Config */ |
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.
What about this message instead. I don't think we need to specify "not just this class's config", it would be better to simply include what this function returns rather than what it doesn't.
/** Provides a way to get the whole config not just this classes' Config */ | |
/** | |
* Get the full RTA config | |
*/ |
return this.config; | ||
} | ||
|
||
public getConfig() { |
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.
It might be worth adding doc comment on each of the getConfig
methods too now that there are two methods, to help clearly call out what each of them does (an to avoid confusion with the new getRtaConfig
method)
public getConfig() { | |
/** | |
* Get the ECP config from the full RTA config. | |
*/ | |
public getConfig() { |
clientSocket.close() | ||
m.clientSockets.delete(messageSocketId) | ||
end if | ||
' We are switching from recursive code to letting us receive another roSocketEvent where we can handle the different parts of the request |
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 comment feels like it belongs in a github pull request comment rather than being in the code itself. The current code knows nothing about being recursive, so it might be worth just documenting what it actually does instead of what it used to do?
Fix socket bug introduced in Roku OS 13.0
Add
getRtaConfig
to allow getting full config from componentsImprove test screenshot file naming