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

Restructure e2e Tests to Improve Organization #979

Merged
merged 1 commit into from
May 19, 2020
Merged

Restructure e2e Tests to Improve Organization #979

merged 1 commit into from
May 19, 2020

Conversation

danielhelfand
Copy link
Member

@danielhelfand danielhelfand commented May 11, 2020 •
edited

This pull request moves e2e tests out of the e2e package and into separate packages based on the subcommand being tested.

Why is this needed?

  • Better organization as far as helping contributors know where to look for/contribute e2e tests with pull requests
  • Make sure tests for one subcommand don't depend on other tests (e.g. currently the task start command relies on helper functions in the pipeline e2e test).
  • Help to set up e2e testing standards to help with the process of building out our e2e test suite and make maintaining these tests easier in the future

My thought is that each subcommand of the cli should have its own package and that test files can follow the naming pattern subcommand_actioncommand_e2e_test.go (e.g. task_start_e2e_test.go). All e2e tests related to tkn task start can then live under that file.

I will be continuing to make future modifications to the e2e suite this release as this pull request doesn't fully address organization issues around our tests, but I think this is a good first step.

Submitter Checklist

  • Includes tests (if functionality changed/added)
  • Run the code checkers with make check
  • Regenerate the manpages, docs and go formatting with make generated
  • Commit messages follow commit message best practices

Release Notes

N/A

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2020
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 11, 2020
@danielhelfand danielhelfand changed the title WIP: Move e2e Tests into Subcommand Packages to Improve Organization WIP: Restructure e2e Tests to Improve Organization May 11, 2020
@danielhelfand danielhelfand changed the title WIP: Restructure e2e Tests to Improve Organization Restructure e2e Tests to Improve Organization May 11, 2020
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2020
vdemeester
vdemeester reviewed May 12, 2020
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Why is this needed?
* Better organization as far as helping contributors know where to look for/contribute e2e tests with pull requests
* Make sure tests for one subcommand don't depend on other tests (e.g. currently the task start command relies on helper functions in the pipeline e2e test).
* Help to set up e2e testing standards to help with the process of building out our e2e test suite and make maintaining these tests easier in the future

I do agree 👏

My thought is that each subcommand of the cli should have its own package and that test files can follow the naming pattern subcommand_actioncommand_e2e_test.go (e.g. task_start_e2e_test.go). All e2e tests related to tkn task start can then live under that file.

So, I don't think we need to add e2 in the name, it's already part of the package path.

test/e2e/e2epipeline/pipeline_e2e_test.go Outdated
@@ -12,7 +13,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package e2e
package e2epipeline
Copy link
Member

Choose a reason for hiding this comment

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

Why not naming the package pipeline (under e2e) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, my original thought was to avoid any confusion with other pipeline package, but I agree and changed it.

test/e2e/e2etask/task_start_e2e_test.go Outdated
@@ -12,27 +13,34 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package e2e
package e2etask
Copy link
Member

Choose a reason for hiding this comment

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

Same here, task

@danielhelfand
Copy link
Member Author

So, I don't think we need to add e2 in the name, it's already part of the package path.

level=warning msg="[runner] Can't run linter goanalysis_metalinter: assign: failed prerequisites: [inspect@github.com/tektoncd/cli/test/e2e/task [github.com/tektoncd/cli/test/e2e/task.test]: analysis skipped: errors in package: [/home/prow/go/src/github.com/tektoncd/cli/test/e2e/task/task_start_e2e_test.go:39:18: undeclared name: Setup]]"
level=error msg="Running error: assign: failed prerequisites: [inspect@github.com/tektoncd/cli/test/e2e/task [github.com/tektoncd/cli/test/e2e/task.test]: analysis skipped: errors in package: [/home/prow/go/src/github.com/tektoncd/cli/test/e2e/task/task_start_e2e_test.go:39:18: undeclared name: Setup]]"
make: *** [Makefile:72: lint-go] Error 3

Appears to fail linting for build tests without e2e. I also noticed would not allow it unless e2e was specified locally, but wasn't sure if I had some IDE setting causing this. If there is a way to do this, more than happy to, but not sure if there is some nuance I am missing.

@vdemeester
Copy link
Member

So, I don't think we need to add e2 in the name, it's already part of the package path.

level=warning msg="[runner] Can't run linter goanalysis_metalinter: assign: failed prerequisites: [inspect@github.com/tektoncd/cli/test/e2e/task [github.com/tektoncd/cli/test/e2e/task.test]: analysis skipped: errors in package: [/home/prow/go/src/github.com/tektoncd/cli/test/e2e/task/task_start_e2e_test.go:39:18: undeclared name: Setup]]"
level=error msg="Running error: assign: failed prerequisites: [inspect@github.com/tektoncd/cli/test/e2e/task [github.com/tektoncd/cli/test/e2e/task.test]: analysis skipped: errors in package: [/home/prow/go/src/github.com/tektoncd/cli/test/e2e/task/task_start_e2e_test.go:39:18: undeclared name: Setup]]"
make: *** [Makefile:72: lint-go] Error 3

Appears to fail linting for build tests without e2e. I also noticed would not allow it unless e2e was specified locally, but wasn't sure if I had some IDE setting causing this. If there is a way to do this, more than happy to, but not sure if there is some nuance I am missing.

golangci-lint is acting weird sometimes.. There is some issue with memory & co too that are being worked-on upstream. I think this might be some falkes ?

@tekton-robot tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 12, 2020
@danielhelfand
Copy link
Member Author

Unfortunately, I keep running into build errors with build tests/integration tests when not using e2e explicitly:

level=warning msg="[runner] Can't run linter goanalysis_metalinter: assign: failed prerequisites: [inspect@github.com/tektoncd/cli/test/e2e/pipeline [github.com/tektoncd/cli/test/e2e/pipeline.test]: analysis skipped: errors in package: [/home/prow/go/src/github.com/tektoncd/cli/test/e2e/pipeline/pipeline_e2e_test.go:47:18: undeclared name: Setup /home/prow/go/src/github.com/tektoncd/cli/test/e2e/pipeline/pipeline_e2e_test.go:48:42: undeclared name: TearDown /home/prow/go/src/github.com/tektoncd/cli/test/e2e/pipeline/pipeline_e2e_test.go:49:8: undeclared name: TearDown /home/prow/go/src/github.com/tektoncd/cli/test/e2e/pipeline/pipeline_e2e_test.go:51:14: undeclared name: NewTknRunner /home/prow/go/src/github.com/tektoncd/cli/test/e2e/pipeline/pipeline_e2e_test.go:80:15: undeclared name: ListAllTasksOutput /home/prow/go/src/github.com/tektoncd/cli/test/e2e/pipeline/pipeline_e2e_test.go:81:8: undeclared name: TaskData /home/prow/go/src/github.com/tektoncd/cli/test/e2e/pipeline/pipeline_e2e_test.go:84:8: undeclared name: TaskData /home/prow/go/src/github.com/tektoncd/cli/test/e2e/pipeline/pipeline_e2e_test.go:99:15: undeclared name: ListAllPipelinesOutput 

test/e2e/pipeline/pipeline_e2e_test.go:47:18: undefined: Setup
test/e2e/pipeline/pipeline_e2e_test.go:48:42: undefined: TearDown
test/e2e/pipeline/pipeline_e2e_test.go:49:8: undefined: TearDown
test/e2e/pipeline/pipeline_e2e_test.go:51:14: undefined: NewTknRunner
test/e2e/pipeline/pipeline_e2e_test.go:80:15: undefined: ListAllTasksOutput
test/e2e/pipeline/pipeline_e2e_test.go:81:8: undefined: TaskData
test/e2e/pipeline/pipeline_e2e_test.go:84:8: undefined: TaskData
test/e2e/pipeline/pipeline_e2e_test.go:99:15: undefined: ListAllPipelinesOutput
test/e2e/pipeline/pipeline_e2e_test.go:100:8: undefined: PipelinesListData

Adding them back for now.

@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 12, 2020
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 17, 2020
@danielhelfand
Copy link
Member Author

So, I don't think we need to add e2 in the name, it's already part of the package path.

@vdemeester Addressed

vdemeester
vdemeester reviewed May 18, 2020
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 18, 2020
pradeepitm12
pradeepitm12 reviewed May 19, 2020
View reviewed changes
Copy link
Contributor

@pradeepitm12 pradeepitm12 left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pradeepitm12

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 19, 2020
@tekton-robot tekton-robot merged commit 8e29999 into tektoncd:master May 19, 2020
@praveen4g0 praveen4g0 mentioned this pull request Jun 9, 2020
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdemeester vdemeester vdemeester left review comments

@pradeepitm12 pradeepitm12 pradeepitm12 approved these changes

@chmouel chmouel Awaiting requested review from chmouel

Assignees

@vdemeester vdemeester

@pradeepitm12 pradeepitm12

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@danielhelfand @vdemeester @tekton-robot @pradeepitm12