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

Add a separate pipeline for integration tests if integration/*.go exists #578

Conversation

garethjevans
Copy link
Contributor

@garethjevans garethjevans commented Feb 28, 2022

Summary

Add a separate pipeline for integration tests if integration/*.go exists.

If this directory does not exist, or does not contain go files then no changes will be applied.

If this directory does exist and contains go files, an Integration Test pipeline will be added. Unit tests will be updated to run with the -short flag so we can avoid running integration tests as part of the unit test pipeline.

Use Cases

I'd like to be able to integration test my buildpack and provide feedback on each PR.

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).

@garethjevans garethjevans requested a review from a team February 28, 2022 16:27
@dmikusa
Copy link
Contributor

dmikusa commented Mar 2, 2022

I'm mixed about how I feel about this. Adding these tools won't break anything, but the tests are something that run often and the install create-package step, in particular, takes 10-15s to run (because it's doing a go build and downloading stuff). So that's an additional 10-15s on every test run for a tool that's not used most of the time.

Cleaning up the create-package install job is on our todo list, but it requires some other work to be done as well. We need to be doing proper releases of create-package and a few other tools, that way they can just be downloaded and installed, like pack (which is fast).

Right now, we add jobs for "Unit Test" and "Create Package Test".

                actions.Job{
			Name:   "Unit Test",
...

I think we should probably add integration tests as a separate actions.Job entry in the workflow. It should also be optionally generated, like if there are actually integration tests defined. That way it's not running anything extra unless the project has integration tests included.


I would also caution about using integration tests, as I mentioned on Slack. Use unit tests to cover as much functionality as possible, and have as small a set of integration tests as possible. They will be slow. Count on 45-60s per integration test. They are going to run on every PR submitted and after every PR is merged, so that can slow down how efficiently you can merge commits (think about when a new version is cut and we have a bunch of PRs to merge for new dependencies).

There are some things you can do to speed up Java integration style tests, we've gotten some of our samples to run in as little as 20-30s.

See this commit and this commit which have details about what we did. Basically, we cache the host maven/gradle repo and volume map it into containers so that each build doesn't have to download all the Maven stuff every time.

@garethjevans
Copy link
Contributor Author

Adding a new optional "Integration Test" job would work well for me, I could configure it based on the presence of an integration package. The issue then would be how to stop those tests from being run as part of the unit test phase. Initial thoughts are that it could be a separate module triggered by richgo, or we could exclude those tests with some golang build tags? what are your thoughts?

@dmikusa
Copy link
Contributor

dmikusa commented Mar 3, 2022

It looks like the pattern the other Paketo projects are using is to have an integration module, like https://github.com/paketo-buildpacks/go-dist/tree/main/integration. Then we could have richgo only run tests against that module. That sounds good to me and keeps things consistent. It also should not require updates to the existing buildpacks that do not include integration tests, which is a plus.

@garethjevans
Copy link
Contributor Author

@dmikusa-pivotal do you think this needs an rfc? It won't take me long to draft one

@dmikusa
Copy link
Contributor

dmikusa commented Mar 3, 2022

I would say no. IMHO, this is internal project details. The RFC would be for more public facing interfaces that we want to guarantee across the project.

@garethjevans
Copy link
Contributor Author

ok thanks, I'll get this PR adjusted in the morning to try and handle that.

@dmikusa dmikusa added type:enhancement A general enhancement semver:minor A change requiring a minor version bump labels Mar 4, 2022
Copy link
Contributor

@dmikusa dmikusa left a comment

Choose a reason for hiding this comment

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

There is also a conflict on the statik.go file. You will probably need to rebase your branch on main & then re-run the go generate to update that. Sorry about that, I had to merge something for our release this week.

Aside from that and my question about -short this all looks good.

octo/run-tests.sh Outdated Show resolved Hide resolved
@garethjevans garethjevans force-pushed the install-pack-and-create-package-for-tests branch 2 times, most recently from 46ce39e to 8d4b08c Compare March 8, 2022 10:35
@garethjevans garethjevans changed the title Install create-package & pack in unit tests phase for occam Add a separate pipeline for integration tests if integration/*.go exists Mar 8, 2022
@garethjevans garethjevans requested a review from dmikusa March 9, 2022 07:06
octo/run-integration-tests.sh Outdated Show resolved Hide resolved
octo/run-short-tests.sh Outdated Show resolved Hide resolved
octo/run-short-tests.sh Outdated Show resolved Hide resolved
octo/test_test.go Show resolved Hide resolved
octo/test_test.go Show resolved Hide resolved
@garethjevans garethjevans force-pushed the install-pack-and-create-package-for-tests branch from 8d4b08c to 712dcb6 Compare March 10, 2022 16:38
@garethjevans garethjevans force-pushed the install-pack-and-create-package-for-tests branch from 712dcb6 to 74fe9d2 Compare March 10, 2022 16:39
@garethjevans garethjevans requested a review from dmikusa March 10, 2022 16:41
@dmikusa dmikusa merged commit 1d21cc3 into paketo-buildpacks:main Mar 11, 2022
@dmikusa
Copy link
Contributor

dmikusa commented Mar 11, 2022

Thanks for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants