-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat(extensions): Add _brs_.testData associative array #646
Conversation
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.
Looks great, just a couple of small nits 😄
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.
Makes sense to me! Can we also add a test case for this?
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.
+1 to test case but changes look good
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 tackling this, @Vasya-M! It looks like _brs_
persists across Interpreter
instances, which may be the motivation behind exposing resetTestData()
outside of this module. I'm curious if we could ensure each Interpreter
instance gets its own copy of _brs_
and solve this problem without having to export a new function that consumers might forget to call!
test/extensions/testData.test.js
Outdated
.get(new BrsString("testData")); | ||
expect(testData.elements.size).toBe(1); | ||
|
||
brs.resetTestData(); // will clear testData for next new Interpreter instances |
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.
Do we want the _brs_.testData
to persist across interpreter
instances? IIRC there's only one created as part of roca
anyway. We'll still want this type of behavior, but perhaps simpler solution would be for each Interpreter
instance to get its own copy of the _brs_
extensions 🤔
src/index.ts
Outdated
@@ -16,6 +16,7 @@ import { | |||
} from "./componentprocessor"; | |||
import { Parser } from "./parser"; | |||
import { Interpreter, ExecutionOptions, defaultExecutionOptions } from "./interpreter"; | |||
export { resetTestData } from "./extensions"; |
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'm not sure that this needs to be exposed from the root of the module. If it does need to be exposed, I'd like for us to find (or add) a new property to put it on instead of it being a sibling of brs.lexer
, brs.parser
, brs.types
, etc. Those are all very low-level concepts, and resetTestData
doesn't match those 😃
@@ -230,6 +231,7 @@ export async function createExecuteWithScope( | |||
interpreter.errors = []; | |||
|
|||
return (filenames: string[], args: BrsTypes.BrsType[]) => { | |||
resetTestData(); |
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 like this solution better than exporting it!
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.
Great idea!
testData
to _brs_
fix for issue #623
added
testData
to_brs_
objectexported new method
resetTestData
for cleaningtestData
before runing test suites(test files)linked roca PR: hulu/roca#88