Skip to content

Conversation

jrichardsz
Copy link

Summary of PR changes

  • Adding unit tests for Node.js.

Questions

  • Should I add code coverage to check for uncovered methods or branches?

Comments

1️⃣ If Node.js tests are included, the Bash tests will function as integration tests.

2️⃣ The build was executed, but since no changes were made to the source code (only tests), Git did not detect it as a change.

build log image

3️⃣ The YAML input input-file: ./env-vars.yml is mandatory, which contradicts what is described in the Markdown.

@jrichardsz jrichardsz requested a review from a team as a code owner September 11, 2025 23:20
Copy link
Contributor

@JosephDSchwartz JosephDSchwartz left a comment

Choose a reason for hiding this comment

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

It would be cool to add a test showing which one, either file or environment variable, takes priority in the case of a variable being define in both. I can't remember which one does, and can't find it in the README, so it might be worth updating that too.

That said, this comment and my other comment aren't super important, so I'll let you decide if you want to do anything about them. I'll approve for now. If you need a re-review after making any updates, feel free to reach out to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

The existence of config_02.yml makes me think there is a config.yml or config_01.yml, so it's a little weird, but not a problem strictly speaking.

Choose a reason for hiding this comment

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

You are right. I will use the base config.yml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants