-
Notifications
You must be signed in to change notification settings - Fork 14
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
Port to Node JS #31
Port to Node JS #31
Conversation
Whoops, never made this clear but since we're doing node, we should look into going containerless. I've tested out that our code would work, most of my question was whether we'd be able to write files on the test runner, and it seemed to go well |
@RGonzalezTech I had a branch I started that had that config if you'd like to add to this one. Also I see you're commiting project lock, if were doing that we'd want to use npm ci and start a dependabot service. |
Dockerfile
I'll update the dockerfile to use Containerless
How would you feel about switching to containerless in another PR/version? This seems to work fine and it has the benefit of managing all of the dependencies separate from any project we'd be interacting with. DependabotI've never set up Dependabot before, but I can take a look at that, too. Do you have any helpful resources for me or could you branch off of this PR branch to add it? I've got some work to do today, but I'll check this stuff out Soon™ haha |
Updated the Dockerfile to use the ubuntu image and install node via RE Dependabot - I don't have access to the settings for this repository, so I don't know if I can turn it on or off. I see that it's enabled for alerts in the Security tab, though. Let me know if this is good to merge & release 👍 |
Okay looks good to me. I'll add an issue to specifically do the swap to containerless. I'll also add an issue to add the .gitattributes so we're ignoring the test directories for releases, and in the containerless PR, I'll have a step for release / tests to include the package bundling process |
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.
That's uhh a lot of files. Doesnt seem necessary to me, but the works already been done. LGTM
Summary
Updated the
Dockerfile
to use a node image and wrote the behavior using Node.JS +jest
for TDD. I also added a github action that will run the jest tests on PRs and pushes tomaster
. (.github/workflows/node-tests.yml
).index.js
is the entrypoint. We use:yargs
package to parse CLI input flags.fast-xml-parser
package to parse the result XMLdecompress
package to unzip the download.The tests cover a edge-cases and most branching logic.
I'll resolve the .net install on docker Soon™
I was developing on a branch that didn't include the Dockerfile changes from 4e8e966 and only recently rebased it😅
Quick Code Notes
It's basically the exact same behavior as before, but I am making more use of functions and unit tests to make sure behavior is covered.
getCliArgs()
- returns parsed arguments from the command line using theyargs
library.downloadGodot()
returns the path to the executableanalyzeTestResults()
uses thefast-xml-parser
package to parse the result XML. This package only has 1 dependency.test_fixtures/testResults.xml
to make sure our code is robust enough to handle it.Related Issue
Closes #28