Adding acceptance tests for nodejs Stack

[pacostas]

Merge After

• Adding BP_UBI_RUN_IMAGE_OVERRIDE environment variable paketo-buildpacks/ubi-nodejs-extension#76 and a new ubi-nodejs-extension release is out.

Related issues

• Remaing steps to complete stack #2

Summary

This PR adds acceptance tests for Node stacks. That way we ensure before each release, that stacks work properly with ubi-nodejs-extension.

Use Cases

Checklist

• I have viewed, signed, and submitted the Contributor License Agreement.
• I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
• I have added an integration test, if necessary.
• I have reviewed the styleguide for guidance on my code quality.
• I'm happy with the commit history on this PR (I have rebased/squashed as needed).

[mhdawson]

Is there a reason to move buildpack_integration_test.go into the subdirectory. Just asking as it makes it less consistent with other stacks, for example - https://github.com/paketo-buildpacks/jammy-full-stack

[mhdawson]

@pacostas Can you confirm that the updates in scripts/.util/tools.sh are just to bring it up to date with the template shared version of that?

[mhdawson]

I think keep in the simple go test might also make sense. If we ever have to temparily disable the Node.js tests it would still be good to have that basic test in place.

[pacostas]

Is there a reason to move buildpack_integration_test.go into the subdirectory. Just asking as it makes it less consistent with other stacks, for example - https://github.com/paketo-buildpacks/jammy-full-stack

@mhdawson They named the file buildpack_integration_test.go and I thought why not putting it under the integration folder following similar pattern to what we have on the buildpacks. BTW I should have remove string integration from the name nodejs_stack_integration_test.go as the file is moved under the integration directory.
Usually on the top level they put the unit tests. The correct I think is to have all the integration tests under the integration directory. In case we put the file under the root directory, we should also move the init_test.go on the root directory. What do you think?

[pacostas]

@pacostas Can you confirm that the updates in scripts/.util/tools.sh are just to bring it up to date with the template shared version of that?

@mhdawson Its not fully aligned yet. I'm working on it.

Status updated: @mhdawson now its fully aligned.

[mhdawson]

Is there a reason to move buildpack_integration_test.go into the subdirectory. Just asking as it makes it less consistent with other stacks, for example - https://github.com/paketo-buildpacks/jammy-full-stack

@mhdawson They named the file buildpack_integration_test.go and I thought why not putting it under the integration folder following similar pattern to what we have on the buildpacks. BTW I should have remove string integration from the name nodejs_stack_integration_test.go as the file is moved under the integration directory. Usually on the top level they put the unit tests. The correct I think is to have all the integration tests under the integration directory. In case we put the file under the root directory, we should also move the init_test.go on the root directory. What do you think?

My main concern is that we can share as much as possible with the generic files like test.sh as possible. If they run something called X then ideally we should have something called X instead of a differently located/named file. Having said that, if we have X and it calls runs/tests that are in other files that is good as well. In this case I thing other stacks had a file called buildpack_integration_test.go so wanted to be consitent with the other stacks.

[pacostas]

Is there a reason to move buildpack_integration_test.go into the subdirectory. Just asking as it makes it less consistent with other stacks, for example - https://github.com/paketo-buildpacks/jammy-full-stack

@mhdawson They named the file buildpack_integration_test.go and I thought why not putting it under the integration folder following similar pattern to what we have on the buildpacks. BTW I should have remove string integration from the name nodejs_stack_integration_test.go as the file is moved under the integration directory. Usually on the top level they put the unit tests. The correct I think is to have all the integration tests under the integration directory. In case we put the file under the root directory, we should also move the init_test.go on the root directory. What do you think?

My main concern is that we can share as much as possible with the generic files like test.sh as possible. If they run something called X then ideally we should have something called X instead of a differently located/named file. Having said that, if we have X and it calls runs/tests that are in other files that is good as well. In this case I thing other stacks had a file called buildpack_integration_test.go so wanted to be consitent with the other stacks.

Aligned also with what exists on jammy implementation, in terms of file structure.

[pacostas]

@mhdawson Ive finished with the review fixes, I think I've covered all suggestions. Can you have a fresh look and see if something is missing, or if the structure is consistent enough?

[pacostas]

Related issue to close after merging this PR #2

[pacostas]

@mhdawson I also added a .github/dependabot.yml file to update the go modules as they were outdated. This file is missing as is not also on the jammy-base-stack, although is present on bionic-base-stack (deprecated stack)
I dont see the reason why not upgrading the go modules, at first i thought that we might have releases very often as new prs for upgrading the go modules will land, but this is not the case as we only have a release when the image hash codes change.

[mhdawson]

@mhdawson I also added a .github/dependabot.yml file to update the go modules as they were outdated. This file is missing as is not also on the jammy-base-stack, although is present on bionic-base-stack (deprecated stack)
I dont see the reason why not upgrading the go modules, at first i thought that we might have releases very often as new prs for upgrading the go modules will land, but this is not the case as we only have a release when the image hash codes change.

Thanks, just sounds like an oversight on my part.

[mhdawson]

@pacostas it still seems to differ in the top level name of the integration test. In the jammy base stack:buildpack_integration_test.go

verus being changed to nodejs_stack_integration_test.go in this PR

[mhdawson]

I'm not sure removing the npm buildpack is good. We likely want to be testing that our run images include both the node and npm binaries. You ready build a buildpack, what would the issue being to just have it include the top level Node.js buildpack and the modified extention (to have the right run images) and use that to build/run the application, making sure that npm is used? I think in that case we would not need the build plan at all ?

[mhdawson]

@pacostas did another run through and left a few more comments

[pacostas]

@pacostas it still seems to differ in the top level name of the integration test. In the jammy base stack:buildpack_integration_test.go

verus being changed to nodejs_stack_integration_test.go in this PR

Yes, I did that intentionally to separate the nodejs stacks from the java stacks or any other stacks that are going to be added in the future, I thought is nice to have them in a separate file. Also I thought the name is little bit misleading as we are not testing buildpacks nor extensions but stacks. This doesnt affect how the integration tests are being called, as they are under the same namespace package acceptance_test. We can also sync about that on todays' meeting.

[pacostas]

I'm not sure removing the npm buildpack is good. We likely want to be testing that our run images include both the node and npm binaries. You ready build a buildpack, what would the issue being to just have it include the top level Node.js buildpack and the modified extention (to have the right run images) and use that to build/run the application, making sure that npm is used? I think in that case we would not need the build plan at all ?

I think is better to sync on that on todays' call, as for some reason the extension does not work without the build plan.

[mhdawson]

@pacostas it still seems to differ in the top level name of the integration test. In the jammy base stack:buildpack_integration_test.go

verus being changed to nodejs_stack_integration_test.go in this PR

Ok understand based on discussion today, tests are run based on package versus the name so naming this way is good.

[pacostas]

I'm not sure removing the npm buildpack is good. We likely want to be testing that our run images include both the node and npm binaries. You ready build a buildpack, what would the issue being to just have it include the top level Node.js buildpack and the modified extention (to have the right run images) and use that to build/run the application, making sure that npm is used? I think in that case we would not need the build plan at all ?

I think is better to sync on that on todays' call, as for some reason the extension does not work without the build plan.

Based on todays discussion, I'll add the buildpacks we usually use to build an app, as we dont want to have unexpected errors coming from extension/buildpack combinations that we are not fully aware.

[mhdawson]

LGTM

[mhdawson]

LGTM