Feature: Add npm test command during npm build

[saraiyakush]

Issue #, if available: aws/aws-sam-cli#7348

Description of changes:

• Add a new NodejsNpmTestAction that runs the npm run test command, if a test script is available in package.json.
• The change is added to make sure tests are executed while running sam build command for Node projects.
• The change will not break any build if a test script isn't available.

CAUTION: I want to make this change non breaking hence added --if-present in the command. However, if any existing Node project has the test script, this feature will start running the tests during its build. Now I do not know if the any version of sam cli will use this latest version of lambda builders once this PR is merged or clients will have to upgrade to a new sam cli. If clients will have to upgrade then we're good as any existing sam build won't break even if they have the test script in their package.json. But if any version of sam cli will use the latest version of lambda builder then clients who do not expect the test script to run will see it run. I need your confirmation for this behaviour. Depending on that, we may have to add some mechanism for the clients to specify if they want this action to take place for their project or not.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[saraiyakush]

Hello, I can see some of the windows java builds have failed. Although I do not know why as the changes are limited to Node. Can it be possible these failures existed before the changes in this PR?

[hnnasit]

Hi @saraiyakush, thanks for the contributions. We will investigate why the tests are failing, they don't look related to your changes.

[lucashuy]

But if any version of sam cli will use the latest version of lambda builder then clients who do not expect the test script to run will see it run.

Newer versions of SAM CLI will utilize the latest version of Lambda Builders that is uploaded on PyPi at the time of creating that new SAM CLI version.

Since this is something we do automatically, clients will consume this change when the following happens:

1. release a new version of Lambda Builders
2. consume that new version in SAM CLI
3. release a new version of SAM CLI

Will this change any implementation details, or will raise up any discussions you had in mind?

[saraiyakush]

But if any version of sam cli will use the latest version of lambda builder then clients who do not expect the test script to run will see it run.

Newer versions of SAM CLI will utilize the latest version of Lambda Builders that is uploaded on PyPi at the time of creating that new SAM CLI version.

Since this is something we do automatically, clients will consume this change when the following happens:

1. release a new version of Lambda Builders
2. consume that new version in SAM CLI
3. release a new version of SAM CLI

Will this change any implementation details, or will raise up any discussions you had in mind?

From what I understood from your response, I don't think we'd have a problem. But let me validate my understanding.

I am a client using SAM CLI let's say version v2.6.1, which points to Lambda builders v4.5. With this PR merged, we get a new lambda builders v4.6.
The SAM CLI v2.6.1 will continue using lambda builders v4.5 and will not see this change.
New SAM CLI v2.6.2 will use lambda builders v4.6 and see this change.

So until a client does not upgrade their local sam cli version (or in their pipelines), we don't have a problem. Most of the pipelines I have seen don't upgrade cli versions.

However, thinking out loud, if any client has some script that installs sam cli each time (e.g. Dockerfile) in their pipelines (highly unlikely), then I assume they will install the latest SAM CLI which will point to the latest lambda builder. So adding a feature flag in this PR may not be a bad idea. So clients can choose if they want to switch on this feature.

What do you think?

[hawflau]

So adding a feature flag in this PR may not be a bad idea. So clients can choose if they want to switch on this feature.

@saraiyakush I'd agree with this suggestion

[saraiyakush]

Cool. I will work on it and add a commit to this PR.

[mndeveci]

Hi @saraiyakush just checking if you had a chance to work on the changes discussed above. Please let us know when we can continue the review.

Thanks again for your contributions!

[saraiyakush]

Hi @mndeveci , unfortunately, I haven't got a chance to add the feature flag further. I also anticipate adding the feature flag via CLI will introduce a change in the sam cli as well, in addition to lambda builders. I have never explored sam cli source code so that will also need to be considered. Nevertheless, I plan to work on this in the next few weeks.

[saraiyakush]

Summarising the need for a feature flag and where I am stuck to implement it.

If we don't add a feature flag, clients who have a script named test in their package.json will see the script being executed as part of sam build.

To allow clients to choose if they want to execute the script test as part of sam build command, clients should have the ability to specify that. I am thinking the following possibilities:

• Passing an argument to the sam build command (e.g. runTests=true), but this will involve a change to the sam cli project and since the change is specific to Node only, I don't think this approach is a good idea.
• Add a parameter in the template.yaml file. Again a change needed in the sam cli project.
• Allow clients to specify another script in package.json that will instruct the builder whether to run the test script or not. Exploring this approach at the moment.

[mndeveci]

Allow clients to specify another script in package.json that will instruct the builder whether to run the test script or not. Exploring this approach at the moment.

Hey @saraiyakush did you have a chance to explore the option above that you mentioned?

[saraiyakush]

Allow clients to specify another script in package.json that will instruct the builder whether to run the test script or not. Exploring this approach at the moment.

Hey @saraiyakush did you have a chance to explore the option above that you mentioned?

Hi @mndeveci , yes. I was exploring many options.

• Existence of a predefined script package.json that would instruct the builder to run the tests
  • This did not work out due to a limitation of NPM that it does not have a way to check if a script exists
• Existence of a predefined environment variable that would instruct the builder to run the tests
  • This seems to be working. But it isn't the most elegant solution.
• Existence of a predefined field in template.yaml
  • This will be the cleanest but it increases the scope of the change beyong lambda builders.

I will update the PR with Option 2 in a few days. But I would love some advise.

[saraiyakush]

Hello,

I've introduced an environment variable SAM_NPM_RUN_TEST_WITH_BUILD (for the lack of a better name) which if set to true will run the command npm run test. This will allow for backward compatibility with all the sam build commands that have the test script but do not want to run it right away.

Teams will have to set this environment variable in order for sam build to run npm tests along with build.

Please review.