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: Add back BATS for testing images #1588

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MasonM
Copy link
Contributor

@MasonM MasonM commented Oct 21, 2021

Description

BATS was initially added to this repository in #802, but was then removed in #1339. This adds it back, and hooks it up to Github Actions.

This also fixes #1583, which happened due to a bug in the "Build image" step: the build context was set to the root project directory, which meant the COPY docker-entrypoint.sh /usr/local/bin/ instruction was copying the base docker-entrypoint.sh file into the Docker image instead of the one in the variant directory. Changing the context to the variant directory solves that.

Motivation and Context

#1579 (comment)

Testing Details

Link to run for this PR: https://github.com/nodejs/docker-node/actions/runs/1374569169

Example Output(if appropriate)

N/A

Types of changes

  • Documentation
  • Version change (Update, remove or add more Node.js versions)
  • Variant change (Update, remove or add more variants, or versions of variants)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Others (non of above)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING.md document.
  • All new and existing tests passed.

.gitmodules Outdated Show resolved Hide resolved
@MasonM MasonM force-pushed the add-back-bats branch 2 times, most recently from 01c1bce to 67017f6 Compare October 23, 2021 02:48
BATS was initially added to this repository in nodejs#802, but was then
removed in nodejs#1339. This adds it back, and hooks it up to Github Actions.

This also fixes nodejs#1583, which happened due to a bug in the "Build image"
step: the build context was set to the root project directory, which
meant the `COPY docker-entrypoint.sh /usr/local/bin/` instruction was
copying the base `docker-entrypoint.sh` file into the Docker image
instead of the one in the variant directory. Changing the context to the
variant directory solves that.

- name: Test for npm
run: docker run --rm node:${{ matrix.version }}-${{ matrix.variant }} npm --version
context: ./${{ steps.short-version.outputs.result }}/${{ matrix.variant }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the fix for issue #1583, in case it wasn't clear from the PR description. The context should be the variant directory so it copies the right docker-entrypoint.sh file.

@MasonM MasonM marked this pull request as ready for review October 23, 2021 03:06
@MasonM MasonM requested a review from SimenB October 27, 2021 18:13
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

I think this makes sense, thanks!

@SimenB SimenB requested a review from a team October 28, 2021 08:16
Copy link
Member

@nschonni nschonni left a 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 I see the value of adding back BATS for these tests. If we do add it back, I think using the old install apt-install method would be better than a submodule.
I think added your other tests the same way as the others currently are there would make sense though

@MasonM
Copy link
Contributor Author

MasonM commented Oct 28, 2021

@nschonni

If we do add it back, I think using the old install apt-install method would be better than a submodule.

Unfortunately, the version of "bats" in Ubuntu 20.04 is quite old and lacks support for the --verbose-run flag. I'm using that flag in this PR because it makes it easier to debug. This is one way BATS is superior to the old method: when tests fail, it clearly shows the expected output and the actual output, without any extra work.

I guess I could have Github Actions download the BATS source with curl and extract it, like this: https://github.com/hyperupcall/basalt/blob/d60b8a101ed3d9c2bb3cf6ae27286d944d03779c/.github/workflows/ci.yml#L35-L42

But that's more complicated than using a submodule.

@SimenB
Copy link
Member

SimenB commented Feb 28, 2022

I think using a submodule makes sense since that's what the official docs recommend: https://bats-core.readthedocs.io/en/stable/tutorial.html#quick-installation.

I know I suggested it in #1588 (comment), but since it has no dependencies we could also just run it through npm exec. As long as we specify a version I don't see any security risk. It is up to date with 1.6.0 released 4 days ago, and maintained by the same person (I assume it's auto-published somehow, haven't dug into i)

@MasonM
Copy link
Contributor Author

MasonM commented Mar 1, 2022

@SimenB Okay, I removed the submodule and changed it to use npx [email protected] instead.

@SimenB SimenB requested a review from a team March 1, 2022 09:08
@chorrell
Copy link
Contributor

Is this still pending another review?

@SimenB
Copy link
Member

SimenB commented Aug 17, 2022

I would like one 😀

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 2 changed files in this pull request and generated no suggestions.

Files not reviewed (1)
  • test/docker-image.bats: Language not supported
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.

Test for execute bit on docker-entrypoint.sh
4 participants