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 busted unit testing framework for lua code #2379

Merged
merged 1 commit into from
Apr 23, 2018

Conversation

zrdaley
Copy link
Contributor

@zrdaley zrdaley commented Apr 19, 2018

What this PR does / why we need it:

Currently Lua code is only tested through e2e tests and no unit tests exist.

This adds busted as a unit testing framework for Lua code and adds documentation within the developer guide.

Which issue this PR fixes: N/A

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 19, 2018
@zrdaley
Copy link
Contributor Author

zrdaley commented Apr 19, 2018

/assign @bowei

@codecov-io
Copy link

codecov-io commented Apr 19, 2018

Codecov Report

Merging #2379 into master will increase coverage by 0.19%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2379      +/-   ##
==========================================
+ Coverage   40.72%   40.91%   +0.19%     
==========================================
  Files          74       74              
  Lines        5228     5252      +24     
==========================================
+ Hits         2129     2149      +20     
- Misses       2807     2809       +2     
- Partials      292      294       +2
Impacted Files Coverage Δ
internal/file/bindata.go 60.17% <83.33%> (+2.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62895ff...4f98655. Read the comment docs.

@aledbf
Copy link
Member

aledbf commented Apr 19, 2018

/assign @ElvinEfendi

@ElvinEfendi
Copy link
Member

@zrdaley why did bindata change?

@zrdaley
Copy link
Contributor Author

zrdaley commented Apr 19, 2018

@ElvinEfendi it seems like all lua changes require bindata to be updated? Or at least anytime new files/directories are added. In this case adding rootfs/etc/nginx/lua/test/balancer-test.lua and rootfs/etc/nginx/lua/test/up.sh seemed make changes.

@ElvinEfendi
Copy link
Member

    go-bindata -nometadata -o internal/file/bindata.go -prefix="rootfs" -pkg=file -ignore=Dockerfile -ignore=".DS_Store" rootfs/...

yeah looks like any update under rootfs, for some reason I thought it is only nginx.tmpl that requires it

@ElvinEfendi
Copy link
Member

/approve

@aledbf
Copy link
Member

aledbf commented Apr 19, 2018

This is required to avoid polluting the filesystem when we run the tests. Before go-bindata it was required to run as root

$ make lua-test
```

Lua tests are located in `$GOPATH/src/k8s.io/ingress-nginx/rootfs/etc/nginx/lua/test`. When creating a new test file it must follow the naming convention `<mytest>-test.lua` or it will be ignored.
Copy link
Member

@ElvinEfendi ElvinEfendi Apr 19, 2018

Choose a reason for hiding this comment

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

can we enforce _test pattern rather than -test?

Copy link
Contributor Author

@zrdaley zrdaley Apr 19, 2018

Choose a reason for hiding this comment

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

Unfortunately no. Initially this is the pattern I used. However, this causes errors because these files are converted to go source code in the bindata.go file and underscores are not allowed in go naming convention. It was causing the ./hack/verify-gofmt.sh test to fail.

Copy link
Member

@ElvinEfendi ElvinEfendi Apr 19, 2018

Choose a reason for hiding this comment

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

Hmm, are you sure? Looking at https://github.com/kubernetes/ingress-nginx/blob/master/hack/verify-gofmt.sh#L32 bindata.go should not be checked by ./hack/verify-gofmt.sh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and that's the problem. go-bindata creates functions for each converted lua file in bindata.go (https://github.com/Shopify/ingress/blob/2b6696573c77a32cc3f4ba3f333e39b5bd746bbd/internal/file/bindata.go#L170). So lua files here can't be named with underscores in them or it will create a go function with an underscore which isn't allowed by the formatter.

@zrdaley zrdaley force-pushed the lua-unit-test branch 4 times, most recently from 3369700 to 382f7ae Compare April 20, 2018 14:11
@@ -23,7 +23,7 @@ KUBE_ROOT=$(dirname "${BASH_SOURCE}")/..
cd "${KUBE_ROOT}"

GOLINT=${GOLINT:-"golint"}
PACKAGES=($(go list ./... | grep -v /vendor/))
PACKAGES=($(find . -type f -name "*.go" | grep -v -E '(vendor/|internal/file/bindata.go)'))
bad_files=()
for package in "${PACKAGES[@]}"; do
out=$("${GOLINT}" -min_confidence=0.9 "${package}" | grep -v 'should not use dot imports' || :)
Copy link
Member

Choose a reason for hiding this comment

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

@zrdaley you could just do something like following?

out=$("${GOLINT}" -min_confidence=0.9 "${package}" | grep -v '(should not use dot imports|internal/file/bindata.go)' || :)

Copy link
Member

Choose a reason for hiding this comment

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

it is not too different than your fix, but with your changes the variable name PACKAGES is not correct anymore

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 20, 2018
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 20, 2018
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 20, 2018
@aledbf
Copy link
Member

aledbf commented Apr 21, 2018

/approve

@aledbf
Copy link
Member

aledbf commented Apr 21, 2018

@zrdaley please rebase

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 21, 2018
@aledbf
Copy link
Member

aledbf commented Apr 21, 2018

@ElvinEfendi add your lgtm when you think this is ready to go

@ElvinEfendi
Copy link
Member

waiting for the rebase

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 23, 2018
@ElvinEfendi
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi, zrdaley

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit b9e1961 into kubernetes:master Apr 23, 2018
@ElvinEfendi ElvinEfendi deleted the lua-unit-test branch April 23, 2018 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants