Test PRs with smoke tests #83

fg-j · 2021-02-22T16:49:54Z

Summary

PRs will have tests run for all latest Paketo builders for any language family subdirectories that contain changes.

The .github/workflows/test-pull-request-<language>.yml workflows also indirectly document which builders are compatible with which language families.

Also fixes a small bug in smoke.sh that caused an unbound variable error when no --builder was provided.

Use Cases

Easily test all sample apps against the builders they should work with
Test PRs to ensure samples are always working

Checklist

I have viewed, signed, and submitted the Contributor License Agreement.
I have linked at least 1 issue to this pull request
I have added an integration test, if necessary.

also adds a workflow to run the tests against PRs and fixes a small bug in smoke.sh that causes a bash error when no --builder is provided

fg-j · 2021-02-22T16:54:16Z

I used the strategy of inspecting the builder with pack rather than inferring based on the builder name to open up the option for someone to have something like testbuilder on their Docker daemon that's a local instance of the full builder, for instance. The tests will still figure out to run the correct tests. This will be helpful for testing builders w/ sample apps directly.

ForestEckhardt

There are still structures at the top of many of the test files that cause the file to be skipped if it is the wrong builder. Could we get rid of all of those now that we have the switch cases in init?

sophiewigmore · 2021-02-22T19:16:29Z

Should all of the smoke tests run on a PR? They take a very long time (especially Java tests, and they are occasionally flaky) and seems like adding a sample app in Node (for example) wouldn't have an effect on any of the other samples. Is there a way we can just run the language family tests that were added?

fg-j · 2021-02-22T19:28:25Z

@ForestEckhardt I didn't realize/remember there was that skipping thing. In some ways I think that's a nicer mechanism than the switch statement in the init. Though maybe it's less explicit? Open to others' thoughts. Maybe I'll just have those skip conditionals look at the builder's run image rather than the name.

Update: Will use the switch statement approach because it collects in one place a list of which language families are compatible with which builders.

fg-j · 2021-02-22T19:29:28Z

@sophiewigmore These tests definitely do take a while. I can explore PR eventing to see if we can conditionally run certain tests based on files changed.

in favor of a switch statement in the init test

so that individual languages' tests can be run with go test also adjusts workflow to run tests based on edited paths

fg-j · 2021-02-22T23:21:43Z

@sophiewigmore I've pushed a WIP change that shows one way we could get better granularity on testing
See this run of the Test Pull Request action for an example of how the workflow runs. @ForestEckhardt What are your thoughts on this approach?

If I went this direction, I'd add a suite to each test file and add a step in the job that tests each one. We end up with some duplication of suites that I don't love...

sophiewigmore · 2021-02-23T14:15:56Z

@fg-j Interesting. So we'd need a - name: Test <Language> Samples in the workflow file for each language family. I don't hate that but I agree it's a lot of duplication and set up.. Maybe it's not worth it for faster tests just for the samples repo since we're not constantly doing development on it. Interested to hear Forest's thoughts

fg-j · 2021-02-23T14:26:07Z

@sophiewigmore something else to consider - not particularly related but wanted to capture the context somewhere: We probably don't want to make the smoke tests a required check for the PR to merge. Here's a gist of why, in my opinion:

Suppose we use these smoke tests to test PRs and releases on the builders repos (where that check is required). Now suppose we release a new version of the Ruby buildpack that replaces a deprecated Ruby version with a new one. We'll want to release a builder with the new Ruby buildpack in it. We open a PR to the builder repo that updates the Ruby buildpack version. The PR tests fail because there's a sample in this repo that uses the deprecated Ruby version. So we need to change the sample app to use a supported version of Ruby. We open a PR to this repo that updates the Ruby version in the sample app. The smoke tests fail on this repo because a version of the builder with the latest Ruby buildpack hasn't been released yet. So we can't merge the PR on this repo, either.

Long story short, to avoid this sort of circular dependency, I think the smoke tests should be an automatic but optional check on this repo.

sophiewigmore · 2021-02-23T22:24:49Z

@fg-j Thanks for the explanation. Yeah, I'm in agreement about this being optional.

ryanmoran · 2021-02-23T22:52:01Z

You could also break up the workflow into one per suite, then use the paths feature on pull_request events to trigger the right workflows for the PR: https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#onpushpull_requestpaths.

fg-j · 2021-02-23T22:55:00Z

@ryanmoran That would get us around using the imported github action, but still doesn't really address the sadness that is the duplicated TestSamples and Test suites. Do you have thoughts on that part?

ryanmoran · 2021-02-23T23:37:06Z

@ryanmoran That would get us around using the imported github action, but still doesn't really address the sadness that is the duplicated TestSamples and Test suites. Do you have thoughts on that part?

Could you colocate the tests with the samples they operate on? So, the Go tests in the go directory, and the PHP tests in the php directory. Then, in each test suite you would specify whether it runs for the given builder.

This way, you could run a single or set of suites like go test ./go ./php and run everything with go test ./....

colocates them with language family samples

ForestEckhardt · 2021-02-25T18:46:32Z

@ekcasey I am going to need an approval from you to merge this one I think.

sophiewigmore · 2021-02-26T13:56:00Z

@fg-j I'm not a maintainer here, but these changes look good to me!

ekcasey

I left a couple comments but the only change I think should be blocking is the go version. Other comments are suggestions for improvements that we could make separately.

This changes weren't added with this PR but I wanted to leave two additional pieces of feedback:

I think we should remove the application insights test for java. The app insights buildpack won't run without a binding and the current test isn't running the app insights buildpack at all.
I worry that asserting on the log output is a bit brittle. The built app images has a label that will tell you which buildpacks ran. Maybe we would rather assert on the contents of the label?

ekcasey · 2021-02-25T21:59:54Z

.github/workflows/test-pull-request-dotnet-core.yml

+    - name: Setup Go
+      uses: actions/setup-go@v1
+      with:
+        go-version: 1.14


Why are we using such an old version of go? I think we should use 1.15 to match go.mod or upgrade both to 1.16.

ekcasey · 2021-02-26T14:34:09Z

go/dep/README.md

@@ -11,3 +11,5 @@
 ## Viewing

 `curl http://localhost:8080`
+
+<!-- a trivial change that should trigger the Go test suite -->


Suggested change

ekcasey · 2021-02-26T14:37:06Z

java/java_native_image_test.go

 	"github.com/sclevine/spec"
+	"github.com/sclevine/spec/report"

 	. "github.com/onsi/gomega"
 	. "github.com/paketo-buildpacks/occam/matchers"


Not related to this PR but maybe we can throw a README.md in https://github.com/paketo-buildpacks/occam. I can guess it's purpose from the context but it wouldn't hurt to have a couple sentences describing what occam is for curious parties.

Opened an issue to track this paketo-buildpacks/occam#83

ekcasey · 2021-02-26T14:47:25Z

tests/test_utilities.go

+	var info builderInfo
+	json.Unmarshal(buffer.Bytes(), &info)
+
+	runImage, err := reference.ParseNormalizedNamed(info.LocalInfo.RunImages[0].ImageName)


Would it make more sense to use a combination of io.buildpacks.stack.id and io.buildpacks.stack.mixins to detect compatibility with a given buildpack? Using the run image name would work most of the time but it seems a bit indirect.

There are two practical situations where this might be a problem

If I was using a dev version of the run image, say I was testing which run image changes would be necessary to make java buildpacks on tiny, this wouldn't detect properly.

If someone created their own bionic stack images and builder and wanted to run these tests.

Maybe we don't need to detect the builder type at all. We could just set up the tests such that we don't pass incompatible builder into certain test suites? It's already a bit confusing to me that commands like the following "pass"

./scripts/smoke.sh --suite ruby --builder paketobuildpacks/builder:tiny --builder paketobuildpacks/builder:tiny

Now that the suites are separate, I agree that it's probably easier and clearer to handle compatibility concerns in the test runner workflows.

ekcasey · 2021-02-26T15:03:06Z

java/java_test.go

 	"github.com/paketo-buildpacks/occam"
+	"github.com/paketo-buildpacks/samples/tests"
 	"github.com/sclevine/spec"
+	"github.com/sclevine/spec/report"

 	. "github.com/onsi/gomega"
 	. "github.com/paketo-buildpacks/occam/matchers"


How do you all typically group import statements in the other paketo repos. In the java buildpack repos (and cnb repos) we typically have three groups:

stdlib (e.g. os)

external (e.g. github.com/paketo-buildpacks/occam)

internal (e.g. github.com/paketo-buildpacks/samples/tests)

From this PR and other paketo buildpack repos am guessing you typically combine groups 2&3 but separate dot imports? But we don't seem to be totally consistent about it. No need to change anything here but maybe this is something we should add to the style guide and make consistent accross the project?

Sounds reasonable. @ekcasey can you open an issue on the community repo to propose a change to the style guide?

paketo-buildpacks/community#17

* Buildpack rename * Use BP_NATIVE_IMAGE env var instead of deprecated BP_BOOT_NATIVE_IMAGE * Fixes examples paths broken by paketo-buildpacks/samples#83 Signed-off-by: Emily Casey <[email protected]>

tests try to determine which sample apps should be tested

c3b29aa

also adds a workflow to run the tests against PRs and fixes a small bug in smoke.sh that causes a bash error when no --builder is provided

fg-j requested a review from a team as a code owner February 22, 2021 16:49

fg-j requested review from ForestEckhardt and kvedurmu February 22, 2021 16:49

fg-j linked an issue Feb 22, 2021 that may be closed by this pull request

Smoke tests not run on PR #73

Closed

3 tasks

ForestEckhardt requested changes Feb 22, 2021

View reviewed changes

Frankie Gallina-Jones added 2 commits February 22, 2021 15:19

removes test-skipping in individual test files

0c5375a

in favor of a switch statement in the init test

WIP adds suites for Dotnet and Go tests

bed503e

so that individual languages' tests can be run with go test also adjusts workflow to run tests based on edited paths

add NGINX to set of samples tested with base builder

99e3537

Frankie Gallina-Jones added 2 commits February 24, 2021 16:56

creates test suites for each language family

7aa9143

colocates them with language family samples

separates language family test workflows

f3fdf95

fg-j requested review from a team, ForestEckhardt and ryanmoran February 24, 2021 22:35

Frankie Gallina-Jones added 2 commits February 25, 2021 09:16

supports multiple --suite flags, updates usage output

93b301d

updates README with testing information

480d375

ForestEckhardt approved these changes Feb 25, 2021

View reviewed changes

ForestEckhardt self-requested a review February 25, 2021 18:44

ForestEckhardt approved these changes Feb 25, 2021

View reviewed changes

kvedurmu approved these changes Feb 25, 2021

View reviewed changes

ekcasey suggested changes Feb 26, 2021

View reviewed changes

upgrades to go 1.16

239c625

ekcasey approved these changes Feb 26, 2021

View reviewed changes

Frankie Gallina-Jones added 3 commits February 26, 2021 16:48

don't infer builder type. just run tests against all passed builders

bd57ddc

fixes java native image unit testing workflow

79199bb

fix paths parameter in java test workflow

50770a7

ForestEckhardt merged commit 3a3188f into main Mar 1, 2021

ForestEckhardt deleted the test-prs branch March 1, 2021 15:03

ekcasey mentioned this pull request Mar 11, 2021

Updates native image docs paketo-buildpacks/paketo-website#122

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test PRs with smoke tests #83

Test PRs with smoke tests #83

fg-j commented Feb 22, 2021 •

edited

Loading

fg-j commented Feb 22, 2021

ForestEckhardt left a comment

sophiewigmore commented Feb 22, 2021 •

edited

Loading

fg-j commented Feb 22, 2021 •

edited

Loading

fg-j commented Feb 22, 2021

fg-j commented Feb 22, 2021 •

edited

Loading

sophiewigmore commented Feb 23, 2021

fg-j commented Feb 23, 2021

sophiewigmore commented Feb 23, 2021

ryanmoran commented Feb 23, 2021

fg-j commented Feb 23, 2021

ryanmoran commented Feb 23, 2021

ForestEckhardt commented Feb 25, 2021

sophiewigmore commented Feb 26, 2021

ekcasey left a comment

ekcasey Feb 25, 2021

ekcasey Feb 26, 2021

ekcasey Feb 26, 2021

fg-j Feb 26, 2021

ekcasey Feb 26, 2021

fg-j Feb 26, 2021

ekcasey Feb 26, 2021

fg-j Feb 26, 2021 •

edited

Loading

ekcasey Feb 26, 2021

Test PRs with smoke tests #83

Test PRs with smoke tests #83

Conversation

fg-j commented Feb 22, 2021 • edited Loading

Summary

Use Cases

Checklist

fg-j commented Feb 22, 2021

ForestEckhardt left a comment

Choose a reason for hiding this comment

sophiewigmore commented Feb 22, 2021 • edited Loading

fg-j commented Feb 22, 2021 • edited Loading

fg-j commented Feb 22, 2021

fg-j commented Feb 22, 2021 • edited Loading

sophiewigmore commented Feb 23, 2021

fg-j commented Feb 23, 2021

sophiewigmore commented Feb 23, 2021

ryanmoran commented Feb 23, 2021

fg-j commented Feb 23, 2021

ryanmoran commented Feb 23, 2021

ForestEckhardt commented Feb 25, 2021

sophiewigmore commented Feb 26, 2021

ekcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fg-j Feb 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fg-j commented Feb 22, 2021 •

edited

Loading

sophiewigmore commented Feb 22, 2021 •

edited

Loading

fg-j commented Feb 22, 2021 •

edited

Loading

fg-j commented Feb 22, 2021 •

edited

Loading

fg-j Feb 26, 2021 •

edited

Loading