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.

Sign up for GitHub

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

Jump to bottom

Add a step to verify vendor code before running unit tests. #77

Closed
tejal29 opened this issue Sep 28, 2018 · 3 comments
Closed

Add a step to verify vendor code before running unit tests. #77

tejal29 opened this issue Sep 28, 2018 · 3 comments
Assignees
@bobcatfish
Labels
area/test-infra Issues or PRs related to the testing infrastructure good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.
Milestone
Mid October Demo

Comments

@tejal29
Copy link
Contributor

tejal29 commented Sep 28, 2018

Expected Behavior

On a Branch, we would want developers to run dep ensure, so that unused dependencies are removed.
We can do this by running a script in the checks which runs dep ensure and check if they are any changes to vendor dir.

Actual Behavior

Right now, developers can easily add unused deps and overlook the libs pulled in since they tend to do this

git add vendor/

Steps to Reproduce the Problem

none

Additional Info
@tejal29 tejal29 added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. area/test-infra Issues or PRs related to the testing infrastructure labels Sep 28, 2018
@tejal29
Copy link
Contributor Author

tejal29 commented Sep 28, 2018

This should also run ./hack/update-dep.sh and ./hack/update-code-gen.sh

@bobcatfish bobcatfish self-assigned this Sep 28, 2018
@tejal29
Copy link
Contributor Author

tejal29 commented Sep 28, 2018 •
edited
Loading

We should also run verify-code-gen in CI and error if there any changes to vendor-ed code. https://github.com/grafeas/kritis/blob/master/hack/verify-codegen.sh

and
https://github.com/grafeas/kritis/blob/master/Makefile#L49

@bobcatfish
Copy link
Collaborator

knative projects have a bunch of scripts in hack that get copied around into all the repos for this, and it seems like there are more in the test-infra bash scripts.

verify-codegen.sh is the script that makes sure dep is a no-op.

bobcatfish referenced this issue in bobcatfish/pipeline Sep 28, 2018
@bobcatfish
Make sure dep is a no-op on every PR
a0f10fe 
Literally copy pasted the logic from the build presubmit script:
https://github.com/knative/build/blob/5f0b563ce9064e49530b2e209b17b4d1bd71e99b/test/presubmit-tests.sh#L23
to run `verify-codegen.sh` to make sure that `dep` will be a no-op.
We've been seeing different behaviour from `update-codegen.sh` on
different ppl's machines, but at least this way we can be consistent
with what we see on Prow.

Also copying the code to make sure building is successful (especially
valuable before we have integration test automation; at least stop them
from completely breaking) and verify our licenses.

Fixes #77
@bobcatfish bobcatfish mentioned this issue Sep 28, 2018
Merged
bobcatfish referenced this issue in bobcatfish/pipeline Sep 28, 2018
@bobcatfish
Make sure dep is a no-op on every PR
e72507e 
Literally copy pasted the logic from the build presubmit script:
https://github.com/knative/build/blob/5f0b563ce9064e49530b2e209b17b4d1bd71e99b/test/presubmit-tests.sh#L23
to run `verify-codegen.sh` to make sure that `dep` will be a no-op.
We've been seeing different behaviour from `update-codegen.sh` on
different ppl's machines, but at least this way we can be consistent
with what we see on Prow.

Also copying the code to make sure building is successful (especially
valuable before we have integration test automation; at least stop them
from completely breaking) and verify our licenses.

Also outputting commands as they are run. It's worth noting that we are
not failing the script when a command fails, so when adding unit tests,
etc., we need to be careful to check if the commands actually succeed.

Fixes #77
@bobcatfish bobcatfish added this to the Mid October Demo milestone Sep 28, 2018
knative-prow-robot pushed a commit that referenced this issue Sep 28, 2018
@bobcatfish @knative-prow-robot
Make sure dep is a no-op on every PR (#80)
4da79b9 
* Make sure dep is a no-op on every PR

Literally copy pasted the logic from the build presubmit script:
https://github.com/knative/build/blob/5f0b563ce9064e49530b2e209b17b4d1bd71e99b/test/presubmit-tests.sh#L23
to run `verify-codegen.sh` to make sure that `dep` will be a no-op.
We've been seeing different behaviour from `update-codegen.sh` on
different ppl's machines, but at least this way we can be consistent
with what we see on Prow.

Also copying the code to make sure building is successful (especially
valuable before we have integration test automation; at least stop them
from completely breaking) and verify our licenses.

Also outputting commands as they are run. It's worth noting that we are
not failing the script when a command fails, so when adding unit tests,
etc., we need to be careful to check if the commands actually succeed.

Fixes #77

* Kick off unit tests from prow

This uses the `report_go_test` function which is responsible for
producing unit tests in a format that can be understood by testgrid. We
aren't using test grid yet but I guess why not get ready for it.

At it's core the function calls `"go test -race -v ${args[@]}"` which
has the added bonus of detecting race conditions so why not I guess.
It doesn't do anything with coverage yet tho so we'll have to tackle
that later.

Fixes #78
chmouel pushed a commit to chmouel/tektoncd-pipeline that referenced this issue Jul 9, 2019
@alanfx
Merge pull request tektoncd#77 from openshift/release-next-ci
f1d017a 
🤖 Triggering CI on branch 'release-next' after synching to upstream/master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bobcatfish bobcatfish

Labels
area/test-infra Issues or PRs related to the testing infrastructure good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.
Projects
None yet
Milestone
Mid October Demo
Development

No branches or pull requests
2 participants
@bobcatfish @tejal29