Fix unit test failures with go 1.15 by shuheiktgw · Pull Request #1214 · tektoncd/cli

shuheiktgw · 2020-10-21T00:03:58Z

Changes

It seems from go 1.15, the error message has been slightly changed and it is quoted now. Why don't we use a prefix match to test if the error messages are as expected?

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

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

See the contribution guide
for more details.

none

linux-foundation-easycla · 2020-10-21T00:04:01Z

The committers are authorized under a signed CLA.

✅ Shuhei Kitagawa (4b2f39750b0fbf14b7f18c2c056fc74ef54f3447)

tekton-robot · 2020-10-21T00:04:13Z

Hi @shuheiktgw. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

shuheiktgw · 2020-10-21T00:09:42Z

It seems easycla dies with some JS errors so let me try signing it later again

danielhelfand · 2020-10-21T00:18:57Z

/ok-to-test

danielhelfand

Thanks for the pr @shuheiktgw!

danielhelfand · 2020-10-21T01:16:33Z

Maybe for continuity of the existing tests, it would make sense to add a field to tp (maybe a boolean named hasPrefix). If hasPrefix is true, this new check you have added takes place. If hasPrefix is false, then test.AssertOutput(t, tp.want, err.Error()) is executed.

Additionally, what if this is defined as a helper func in helper.go and followed a similar output format for errors as AssertOutput?

What do you think?

Sounds great! Let me fix the code quickly 😃

danielhelfand · 2020-10-21T01:20:31Z

Another thing to make note of with regard to #1202 is the following error:

pkg/formatted/color_test.go:42:10: conversion from int to string yields a string of one rune, not a string of digits (did you mean fmt.Sprint(x)?)

This is happening in color_test.go and can be resolved as follows:

for c := range palette {
    a := strconv.Itoa(c)
    rb.get(a)
}

shuheiktgw · 2020-10-21T23:52:52Z

I'm looking into color_test.go now

shuheiktgw · 2020-10-21T23:59:47Z

@danielhelfand Thank you for your review! I've fixed the PR so would you review it again? 🙏

danielhelfand

/lgtm

cc @piyush-garg @pradeepitm12

danielhelfand · 2020-10-22T02:53:38Z

@shuheiktgw Could you please also squash your commits into a single commit?

shuheiktgw · 2020-10-22T03:07:43Z

@danielhelfand Sure, I've squashed the commits so please check it!

danielhelfand

/lgtm

@shuheiktgw Thank you! Just needs another review from someone else.

piyush-garg

/lgtm

tekton-robot · 2020-10-22T03:18:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: piyush-garg

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [piyush-garg]

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

#1215 | [Piyush Garg] Update readme for 0.13.1 and fix lint task | 2020/10/21-14:41 #1210 | [Daniel Helfand] common way of referring to tekton resources in user facing messages: ClusterTriggerBinding | 2020/10/21-15:55 #1209 | [Daniel Helfand] common way of referring to tekton resources in user facing messages: Condition | 2020/10/21-17:41 #1213 | [Daniel Helfand] common way of referring to tekton resources in user facing messages: TriggerTemplate | 2020/10/21-19:33 #1214 | [shuheiktgw] Fix unit test failures with go 1.15 | 2020/10/22-04:19 #1203 | [Sunil Thaha] Integrate Hub commands into tkn | 2020/10/22-04:41 #1217 | [Daniel Helfand] fix alignment of usage | 2020/10/22-18:47 #1218 | [Daniel Helfand] document NO_COLOR and edit README headers | 2020/10/26-15:40 #1224 | [Daniel Helfand] change --nocolour to --no-color | 2020/10/28-02:57 #1221 | [SM43] Updates Hub Dependency | 2020/10/28-04:51 null | [Daniel Helfand] deprecate -c shorthand for tkn version --check | 2020/11/03-15:37 null | [Pradeep Kumar] Bump pipeline and triggers deps | 2020/11/04-20:12 null | [Daniel Helfand] do not delete clustertask taskruns with --task flag | 2020/11/04-20:59 null | [Piyush Garg] Fix APIversion not shown on describe commands | 2020/11/05-10:54 null | [Pratik Jagrut] Add --all-namespaces flag to tkn triggerbinding list command | 2020/11/06-12:25 null | [Pratik Jagrut] Add --all-namespaces flag to tkn eventlistener list command | 2020/11/06-16:04 null | [vinamra28] Add --all-namespaces flag to TriggerTemplate List subcommand | 2020/11/06-19:39 null | [Chmouel Boudjnah] Make sure we target LTS for ubuntus | 2020/11/06-21:12 null | [Pratik Jagrut] Add --all-namespaces flag to tkn condition list command | 2020/11/09-14:14 null | [Daniel Helfand] remove EventListener log output assertion from e2e tests | 2020/11/11-16:52 null | [Pradeep Kumar] bump pipleine dependency to v0.18.0 | 2020/11/12-11:28 null | [dullest] fix function name typo | 2020/11/12-15:17 null | [Vincent Demeester] Makefile: remove darwin/386 from cross target 🌮 | 2020/11/13-15:58 null | [Pratik Jagrut] Add --no-headers flag to tkn clustertask list command | 2020/11/16-09:50 null | [Pratik Jagrut] Add --no-headers flag to tkn clustertriggerbinding list command | 2020/11/16-10:24 Signed-off-by: Pradeep Kumar <pradkuma@redhat.com>

tekton-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Oct 21, 2020

tekton-robot requested review from chmouel and sthaha October 21, 2020 00:04

tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 21, 2020

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 21, 2020

danielhelfand suggested changes Oct 21, 2020

View reviewed changes

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 21, 2020

shuheiktgw changed the title ~~Fix unit test failures with go 1.5~~ Fix unit test failures with go 1.15 Oct 21, 2020

shuheiktgw requested a review from danielhelfand October 22, 2020 02:43

danielhelfand reviewed Oct 22, 2020

View reviewed changes

tekton-robot assigned danielhelfand Oct 22, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2020

Fix unit test failures with go 1.15

81b5c40

shuheiktgw force-pushed the fix_go1_5_tests branch from 578e7b2 to 81b5c40 Compare October 22, 2020 03:06

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2020

danielhelfand reviewed Oct 22, 2020

View reviewed changes

tekton-robot added lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. labels Oct 22, 2020

tekton-robot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Oct 22, 2020

piyush-garg approved these changes Oct 22, 2020

View reviewed changes

tekton-robot assigned piyush-garg Oct 22, 2020

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2020

tekton-robot merged commit 755923e into tektoncd:master Oct 22, 2020

Conversation

shuheiktgw commented Oct 21, 2020 • edited by piyush-garg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Uh oh!

linux-foundation-easycla Bot commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tekton-robot commented Oct 21, 2020

Uh oh!

shuheiktgw commented Oct 21, 2020

Uh oh!

danielhelfand commented Oct 21, 2020

Uh oh!

danielhelfand left a comment

Choose a reason for hiding this comment

Uh oh!

danielhelfand Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

shuheiktgw Oct 21, 2020

Choose a reason for hiding this comment

Uh oh!

danielhelfand commented Oct 21, 2020

Uh oh!

shuheiktgw commented Oct 21, 2020

Uh oh!

shuheiktgw commented Oct 21, 2020

Uh oh!

danielhelfand left a comment

Choose a reason for hiding this comment

Uh oh!

danielhelfand commented Oct 22, 2020

Uh oh!

shuheiktgw commented Oct 22, 2020

Uh oh!

danielhelfand left a comment

Choose a reason for hiding this comment

Uh oh!

piyush-garg left a comment

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Oct 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shuheiktgw commented Oct 21, 2020 •

edited by piyush-garg

Loading

linux-foundation-easycla Bot commented Oct 21, 2020 •

edited

Loading