Skip to content
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(runner): Convert test runner from BrightScript to TS #62

Merged
merged 15 commits into from
Feb 19, 2021

Conversation

lkipke
Copy link
Contributor

@lkipke lkipke commented Jan 6, 2021

Change Summary

This converts the functionality from roca_main.brs to Typescript, using the new createExecuteFromScope functionality implemented in sjbarag/brs#603.

Running tests in TS rather than Brightscript gets us much more flexibility with error handling/reporting, plus it allows for much easier future enhancements to CLI args (think: passing in a single file name to test, grep-ing file patterns, etc). And it will integrate nicely with a Jest-style reporter that we will eventually (hopefully) have.

@lkipke lkipke added the enhancement New feature or request label Jan 6, 2021
@lkipke lkipke self-assigned this Jan 6, 2021
src/runner/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@alimnios72 alimnios72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will definitely give us more flexibility in the future, thanks for the hard work here. Other than my comments below I have a question regarding tap.brs: can we convert that file to Typescript in the future? if so, is there any issue tacking that?

"spec",
"tap",
"xunit",
] as const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

src/runner/TestRunner.ts Outdated Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
@lkipke
Copy link
Contributor Author

lkipke commented Feb 11, 2021

This will definitely give us more flexibility in the future, thanks for the hard work here. Other than my comments below I have a question regarding tap.brs: can we convert that file to Typescript in the future? if so, is there any issue tacking that?

I don't think we can convert tap.brs, because each brightscript function calls into it to report test results.

@sjbarag
Copy link
Contributor

sjbarag commented Feb 11, 2021

This will definitely give us more flexibility in the future, thanks for the hard work here. Other than my comments below I have a question regarding tap.brs: can we convert that file to Typescript in the future? if so, is there any issue tacking that?

I don't think we can convert tap.brs, because each brightscript function calls into it to report test results.

That sounds correct! The only alternative is to replace TAP as the brightscript-javascript bridge with some other format of our choosing. I could see a world where it becomes a JSON payload (with lines prefixed by some magic string for easier tagging), or if we wanted to get really creative we could add an extra function to the global _brs_ namespace that directly interacts with the javascript side — maybe with a node event-emitter that roca subscribes to?

That kind of migration seems out of scope for now though, and might not even be necessary if TAP isn't in the way of implementing features.

Copy link
Contributor

@sjbarag sjbarag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all makes sense! Great work @lkipke — I'm looking forward to having more a more sophisticated test API 🧐🧣🍷🎻:high_society:.

I've got a few small suggestions, but I'm approving on the assumption you'll do something reasonable in either direction with them 😉

Comment on lines +27 to +31
/**
* Generates an execution scope and runs the tests.
* @param files List of filenames to load into the execution scope
* @param options BRS interpreter options
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ thanks for adding this!

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/runner/TestRunner.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@alimnios72 alimnios72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the changes

@lkipke lkipke merged commit 298b218 into hulu:main Feb 19, 2021
@lkipke lkipke deleted the convert-roca-main-to-ts branch February 19, 2021 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants