New issue
Advanced search Search tips

Issue 887166 link

Starred by 1 user
Status: Fixed
Owner:
nya@chromium.org
Closed: Oct 12
Cc:
derat@chromium.org
hidehiko@chromium.org
nya@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

Introduce error-handling library

Project Member Reported by nya@chromium.org, Sep 20 
We have a lot of following error handling code:

 if err := foo(...); err != nil {
   return fmt.Errorf("foo failed: %v", err)
 }

This approach has several known problems, e.g. original error type is erased by fmt.Errorf.

There are third-party libraries trying to solve this issue. pkg/errors is the most popular one IIUC:
https://godoc.org/github.com/pkg/errors

It allows to write the above code as:

 if err := foo(...); err != nil {
   return errors.Wrap(err, "foo failed")
 }

It allows extracting original errors, enforces message formatting, records stacktrace, etc.

How about introducing the library? If you have better alternative please comment.

Comment 1 by hidehiko@chromium.org, Sep 20 

Agreed. pkg/errors is actually what I found too and was thinking about to propose.

Comment 2 by derat@chromium.org, Sep 20 

Is losing error types due to wrapping causing problems right now? I'm not sure -- maybe I've had to write ugly code once or twice that compares error strings, but I'm not sure that it was in Tast.

I agree that the current system is non-optimal. I think my main concern about using something like pkg/errors is that there's now a draft design for improving errors in Go 2 (see https://go.googlesource.com/proposal/+/master/design/go2draft-error-values-overview.md and more details at https://go.googlesource.com/proposal/+/master/design/go2draft.md). I'd rather we not switch to a different system if it's going to be made obsolete soon in Go 2.

Comment 3 by nya@chromium.org, Sep 20 

Main advantages I want by adopting a third-party library are:

- Recording stack trace. Currently, testing.NewError records the stack trace, but it only points the Error/Fatal call in test body functions. We usually want stack traces more near to the root cause. But as long as we stick with fmt.Errorf, we can't get them.
- Enforcing message formatting. Newbies like me are not familiar with Go's error formatting rule, so we tend to do something like fmt.Errorf("%s; aborting to connect", err). Using wrapping function like errors.Wrap, we can enforce uniform message formatting.

And of course, programatic error unwrapping is good to have.


Thanks for the pointer to Go 2 error proposal. I read throught it and found it interesting and promising. After reading it, I'm actually optimistic about adopting third-party library.

I have two questions:

1. Do you think adopting pkg/errors (or any other third-party utility) will make the transition to Go 2 error interface more difficult? It's not ovbious to me, and actually I think it's possible the transition will get easier: the upstream can add support of Go 2 errors and then client code will get ready.

2. When will Go 2 come? I could not find an official document mentioning ETA. If it's coming in a few months I agree we can wait it. But if it will be after a year or two, I'm not sure it's good idea to wait.

Comment 4 by derat@chromium.org, Sep 20 

Thanks for the thoughtful discussion. The explanation of wanting to get the root stack trace in test errors makes a lot of sense.

I'm a little worried that Go error reporting is already hard for people new to Go, and that adding a third-party package may make it even more confusing. Hopefully that fear is unfounded, though.

I'm not sure when Go 2 is coming out either.

So I'm okay with us experimenting with pkg/errors. Maybe we could just start using it in tests first?

Also note that I may be slow reviewing code. I currently have enough code review requests and emailed questions that I don't really have time to write code of my own, which makes me sad. :-(

Comment 5 by nya@chromium.org, Sep 21

Status: Started (was: Untriaged)
Thanks Dan! Then let's try experimenting.

> Maybe we could just start using it in tests first?

What did you mean by "tests" here? I guess tast-tests/ code (not tast/ code)? I just wanted to make sure we're on the same page.

Comment 6 by derat@chromium.org, Sep 21 

Yeah, tast-tests is what I meant. If there's benefit to using it in tast as well and it's easy to do, I don't have any objections, though.

Comment 7 by nya@chromium.org, Sep 27 

I've been experimenting with pkg/errors, but I found several difficulties, so I have an alternative proposal.


Problems
--------

First, stack trace generated by pkg/errors is too verbose. See an example below [A]. As you can see, the library captures the full stack trace on every call of errors.Wrap(), so usually outer frames are duplicated, which makes it harder to read even for this simple error.

And unfortunately the library does not provide enough interface to format them nicely. I tried improving this from outside of the library, and only got [B]. It's still not good because of two issues: (i) we can't split error messages at each level because pkg/errors does not export withMessage struct [1]. (ii) One Wrap() call is split to two lines because it encloses an error with withMessage and withStack [2]. These problems are difficult to solve from outside of the library.

Also, pkg/errors provides a few utilities that are unlikely to be provided by proposed Go 2 error library:

 - errors.Cause() [3] has different semantics than what is proposed in Go 2 draft because of some known issues [4].
 - errors.WithMessage() [5] and errors.WithStack() [6] are also unlikely to be adopted.

If we use pkg/errors now, people may use these functions, which will make future migration harder.


Proposal
--------

Considering these problems, I propose to introduce our own errors library (!) instead of adopting pkg/errors.

The new library should be designed to be minimal so that it's easy to migrate to other errors library (maybe that of Go 2) in the future. What we want would be:

 - New: equivalent of errors.New, but records the location
 - Errorf: equivalent of fmt.Errorf, but records the location
 - Wrap/Wrapf: equivalent of those in pkg/errors

That's all. We won't provide Cause[3]/Unwrap[4]/WithMessage[5]/WithStack[6]/Is[7]/As[7]. Maybe we want to consider Is/As if we need in the future, but at this moment we don't have use case (and we can't implement As in Go 1.x).

I know I'm proposing something too ambitious.
Please let me hear your opinion!


References
----------

[1] https://github.com/pkg/errors/blob/master/errors.go#L223
[2] https://github.com/pkg/errors/blob/master/errors.go#L180
[3] https://godoc.org/github.com/pkg/errors#Cause
[4] https://go.googlesource.com/proposal/+/master/design/go2draft-error-values-overview.md#discussion-and-open-questions
[5] https://godoc.org/github.com/pkg/errors#WithMessage
[6] https://godoc.org/github.com/pkg/errors#WithStack
[7] https://go.googlesource.com/proposal/+/master/design/go2draft-error-values-overview.md#error-inspection


Appendix
--------

[A] Stack trace generated by pkg/errors

2018/09/27 16:17:10 [16:17:09.946] Error at chrome_login.go:71: Chrome login failed: OOBE didn't show up (Oobe.readyForTesting not found): context deadline exceeded; last error following: "!!(typeof Oobe == 'function' && Oobe.readyForTesting)" is false
2018/09/27 16:17:10 [16:17:09.946] Stack trace:
"!!(typeof Oobe == 'function' && Oobe.readyForTesting)" is false
chromiumos/tast/local/chrome.(*Conn).WaitForExpr
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/chrome/conn.go:146
chromiumos/tast/local/chrome.(*Chrome).logIn
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/chrome/chrome.go:477
chromiumos/tast/local/chrome.New
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/chrome/chrome.go:172
chromiumos/tast/local/bundles/cros/ui.ChromeLogin
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/ui/chrome_login.go:43
chromiumos/tast/testing.(*Test).Run.func2
        /home/nya/trunk/src/platform/tast/src/chromiumos/tast/testing/test.go:114
runtime.goexit
        /usr/lib/go/x86_64-cros-linux-gnu/src/runtime/asm_amd64.s:2361
context deadline exceeded; last error following
chromiumos/tast/testing.Poll
        /home/nya/trunk/src/platform/tast/src/chromiumos/tast/testing/poll.go:70
chromiumos/tast/local/chrome.(*Conn).WaitForExpr
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/chrome/conn.go:147
chromiumos/tast/local/chrome.(*Chrome).logIn
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/chrome/chrome.go:477
chromiumos/tast/local/chrome.New
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/chrome/chrome.go:172
chromiumos/tast/local/bundles/cros/ui.ChromeLogin
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/ui/chrome_login.go:43
chromiumos/tast/testing.(*Test).Run.func2
        /home/nya/trunk/src/platform/tast/src/chromiumos/tast/testing/test.go:114
runtime.goexit
        /usr/lib/go/x86_64-cros-linux-gnu/src/runtime/asm_amd64.s:2361
OOBE didn't show up (Oobe.readyForTesting not found)
chromiumos/tast/local/chrome.(*Chrome).logIn
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/chrome/chrome.go:478
chromiumos/tast/local/chrome.New
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/chrome/chrome.go:172
chromiumos/tast/local/bundles/cros/ui.ChromeLogin
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/ui/chrome_login.go:43
chromiumos/tast/testing.(*Test).Run.func2
        /home/nya/trunk/src/platform/tast/src/chromiumos/tast/testing/test.go:114
runtime.goexit
        /usr/lib/go/x86_64-cros-linux-gnu/src/runtime/asm_amd64.s:2361

[B] Stack trace generated by my experimental impl using pkg/errors

Chrome login failed: OOBE didn't show up (Oobe.readyForTesting not found): context deadline exceeded; last error following: "!!(typeof Oobe == 'function' && Oobe.readyForTesting)" is false
        at chromiumos/tast/local/bundles/cros/ui.ChromeLogin (/home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/ui/chrome_login.go:71)
OOBE didn't show up (Oobe.readyForTesting not found): context deadline exceeded; last error following: "!!(typeof Oobe == 'function' && Oobe.readyForTesting)" is false
        at (*Chrome).logIn (chrome.go:478)
OOBE didn't show up (Oobe.readyForTesting not found): context deadline exceeded; last error following: "!!(typeof Oobe == 'function' && Oobe.readyForTesting)" is false
        at ???
context deadline exceeded; last error following: "!!(typeof Oobe == 'function' && Oobe.readyForTesting)" is false
        at Poll (poll.go:70)
context deadline exceeded; last error following: "!!(typeof Oobe == 'function' && Oobe.readyForTesting)" is false
        at ???
"!!(typeof Oobe == 'function' && Oobe.readyForTesting)" is false
        at (*Conn).WaitForExpr (conn.go:146)

Comment 8 by hidehiko@chromium.org, Sep 27 

So there are two problems in #7.
- Compatibility with Go 2 error proposal.
- Formatting issue.

If we think the 100% compatibility, I think we need to suspend this, because it is still in draft, and I guess it'd take 0.5 - 1 year to be fixed at least. It means, even if we choose the interface which is well compatible with current Go 2 error draft, in future, they may change. Also, because the proposal depends on the concept, yet another feature of Go 2, we cannot backport it into Go 1.x, directly.

BTW, IMO, practically, limiting error APIs to commonly used ones sounds good to me.
Though, if it means reimplementing whole error libs, it sounds overkill to me, TBH...
Or, do you mean introducing thin wrappers of pkg/errors (or some other 3P libs)?

If formatting is the issue, e.g., I think we can "technically" collapse the common frames for nested errors if we wouldn't choose the implementation. So, something like;

2018/09/27 16:17:10 [16:17:09.946] Error at chrome_login.go:71: Chrome login failed: OOBE didn't show up (Oobe.readyForTesting not found): context deadline exceeded; last error following: "!!(typeof Oobe == 'function' && Oobe.readyForTesting)" is false
2018/09/27 16:17:10 [16:17:09.946] Stack trace:
"!!(typeof Oobe == 'function' && Oobe.readyForTesting)" is false
chromiumos/tast/local/chrome.(*Conn).WaitForExpr
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/chrome/conn.go:146
context deadline exceeded; last error following
chromiumos/tast/testing.Poll
        /home/nya/trunk/src/platform/tast/src/chromiumos/tast/testing/poll.go:70
chromiumos/tast/local/chrome.(*Conn).WaitForExpr
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/chrome/conn.go:147
chromiumos/tast/local/chrome.(*Chrome).logIn
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/chrome/chrome.go:477
OOBE didn't show up (Oobe.readyForTesting not found)
chromiumos/tast/local/chrome.(*Chrome).logIn
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/chrome/chrome.go:478
chromiumos/tast/local/chrome.New
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/chrome/chrome.go:172
chromiumos/tast/local/bundles/cros/ui.ChromeLogin
        /home/nya/trunk/src/platform/tast-tests/src/chromiumos/tast/local/bundles/cros/ui/chrome_login.go:43
chromiumos/tast/testing.(*Test).Run.func2
        /home/nya/trunk/src/platform/tast/src/chromiumos/tast/testing/test.go:114
runtime.goexit
        /usr/lib/go/x86_64-cros-linux-gnu/src/runtime/asm_amd64.s:2361

(we may want to decorate "msg" part for more visibility).

Though, IIUC, the implementation can be slightly tricky in tast (note: not tast-tests. In tast-tests, pkg/errors can be simply used).
# Maybe, some reflection, or some assumption of error formatting by pkg/errors.
Thus, at least it's better to limit the support. (I'm imagining to support only in testing.State.{Error,Fatal}f?() formatting).
The question along with this approach I have is; Do we accept such implementation from tast's maintenance point of view...?

BTW, it's better to think about how to format the stacktrace (mine is just an example/suggestion),
even if you choose the self-implemented new error library approach,
because anyway it is necessary to be formatted somehow.

Comment 9 by derat@chromium.org, Sep 28 

Thanks for the additional in-depth analysis! I haven't used any of these packages before and I'm learning a lot here. :-)

The main motivation for this right now is to improve tast-tests, right? Stack traces are common in tests (i.e. whenever we report a failure), but we generally don't surface them in the tast/runner/bundle code unless there's a panic, AFAIK.

I don't have any objections to introducing our own minimal library for attaching more information to errors, as long as its only impact on test code is that we end up calling different functions instead of fmt.Errorf and errors.New.

Comment 10 by nya@chromium.org, Sep 28 

Thanks for your opinions!

With the current design, I drafted a CL:
https://chromium-review.googlesource.com/c/chromiumos/platform/tast/+/1250623

Would you mind taking a look, and giving your thoughts?

---

> If we think the 100% compatibility, I think we need to suspend this, because it is still in draft, and I guess it'd take 0.5 - 1 year to be fixed at least. It means, even if we choose the interface which is well compatible with current Go 2 error draft, in future, they may change. Also, because the proposal depends on the concept, yet another feature of Go 2, we cannot backport it into Go 1.x, directly.

Of course it's impossible to achieve 100% compatibility. However just waiting for Go 2 is not optimal for us, so this is a trade-off.


> Though, if it means reimplementing whole error libs, it sounds overkill to me, TBH...

As you see in the draft CL, actually implementing the library itself is not a lot of work. I hope this pays enough.


> BTW, it's better to think about how to format the stacktrace (mine is just an example/suggestion),
> even if you choose the self-implemented new error library approach,
> because anyway it is necessary to be formatted somehow.

Yes, I really welcome your opinion about trace formatting. My current approach is to record one frame per Wrap() calls and s.Error() calls, but not deeper.

One requirement for us is that we want good-looking error messages because they show up in our dashboards like Stainless. So we need to wrap errors in every level of callers to add message. This means we have enough invocations of errors.Wrap() in call stacks so we don't need to record deeper frames.


> The main motivation for this right now is to improve tast-tests, right? Stack traces are common in tests (i.e. whenever we report a failure), but we generally don't surface them in the tast/runner/bundle code unless there's a panic, AFAIK.

Yes, exactly. Introducing to tast-tests is the primary goal. It might be useful in core tast code, but it's a different story.


> I don't have any objections to introducing our own minimal library for attaching more information to errors, as long as its only impact on test code is that we end up calling different functions instead of fmt.Errorf and errors.New.

Good to hear that! Please let me hear your opinion about draft CL.

Comment 11 by hidehiko@chromium.org, Sep 28 

One of the goal of this effort I thought is, allowing "return err" issue style to propagate callee's error to the callers, if there's no additional context to be annotated.

Currently, only the call site is recorded.
So, if "return err" stmt is used, the stack trace won't be chained.
Stepping back, if we request authors of tests to write "return errors.Wrap(err)" wherever, it means still manual stack trace, and in case of failure, we can search the code.

If we're introducing a new library, I'd expect to have more complete stack traces, like what pkg/errors have. (Note: I mean the information, but not the formatting itself, which seems to be able to be improved).

So, per offline chat, it seems like we're not yet on the same page from the starting point.
nya@ san, would you mind to summarize;
- what/how you'd like to show the stack, and where.
- what info is needed.
- what code do you expect for test authors to write.
- what's alternative, and why they are not chosen.

etc.? Maybe in this level, it's nice to have small design doc, IMHO.

Comment 12 by derat@chromium.org, Sep 28 

Thanks! I don't have any objections to the draft CL, but I'd also like to see a brief example of what test code would look like while using this.

Comment 13 by nya@chromium.org, Oct 1 

I've drafted another CL with the same API but different trace formatting (full stack traces):
https://chromium-review.googlesource.com/c/chromiumos/platform/tast/+/1253682

Would you mind taking a look? I'd like to hear your opinion about formatting.

Actually I'm in favor of the second formatting. I thought it can be too verbose, but verbosity was reduced by proper indentation and stripping long file paths (usually duplicated with function package names). With this implementation, we can give good traces even with "return err", while it's another discussion if we want to allow the style.


And I'm writing a short design doc and more usage examples. I'll update soon.
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 3 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/9cbd81fbf688ede69a1bffc4420331e45a15fe79

commit 9cbd81fbf688ede69a1bffc4420331e45a15fe79
Author: Shuhei Takahashi <nya@chromium.org>
Date: Wed Oct 03 20:59:17 2018

tast-bundle.eclass: Lint Wrap/Wrapf calls.

CL:1253682 introduced errors.{Wrap,Wrapf} functions which can be
checked by -printfuncs.

BUG= chromium:887166 
TEST=fast_build.sh -T -C

Change-Id: Ia7195fecdcdf5321df87d39a6987607de5b03df3
Reviewed-on: https://chromium-review.googlesource.com/1258643
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>

[modify] https://crrev.com/9cbd81fbf688ede69a1bffc4420331e45a15fe79/eclass/tast-bundle.eclass
Project Member

Comment 15 by bugdroid1@chromium.org, Oct 3 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/9c9817e583f9f76abda5bc6753a4fe5e5b8d9596

commit 9c9817e583f9f76abda5bc6753a4fe5e5b8d9596
Author: Shuhei Takahashi <nya@chromium.org>
Date: Wed Oct 03 20:59:29 2018

tast: Introduce our own errors package.

This CL introduces chromiumos/tast/errors package which provides
very basic building block to create errors with rich stack traces.

testing.State methods are updated to extract errors from their
arguments and show stack traces.

Also updates some utility functions used from tests to use the
new errors library.

BUG= chromium:887166 
TEST=fast_build.sh
TEST=fast_build.sh -T
TEST=tast run DUT ui.ChromeLogin

Change-Id: I00ad50fe64ba0fbd28d50f1083f76e3c5197cf88
Reviewed-on: https://chromium-review.googlesource.com/1253682
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>

[add] https://crrev.com/9c9817e583f9f76abda5bc6753a4fe5e5b8d9596/src/chromiumos/tast/errors/errors_test.go
[add] https://crrev.com/9c9817e583f9f76abda5bc6753a4fe5e5b8d9596/src/chromiumos/tast/errors/stack/stack_test.go
[add] https://crrev.com/9c9817e583f9f76abda5bc6753a4fe5e5b8d9596/src/chromiumos/tast/errors/stack/stack.go
[add] https://crrev.com/9c9817e583f9f76abda5bc6753a4fe5e5b8d9596/src/chromiumos/tast/errors/errors.go
[modify] https://crrev.com/9c9817e583f9f76abda5bc6753a4fe5e5b8d9596/src/chromiumos/tast/testing/state.go
[modify] https://crrev.com/9c9817e583f9f76abda5bc6753a4fe5e5b8d9596/fast_build.sh
[modify] https://crrev.com/9c9817e583f9f76abda5bc6753a4fe5e5b8d9596/src/chromiumos/tast/testing/poll.go
[modify] https://crrev.com/9c9817e583f9f76abda5bc6753a4fe5e5b8d9596/src/chromiumos/tast/testing/state_test.go
[modify] https://crrev.com/9c9817e583f9f76abda5bc6753a4fe5e5b8d9596/src/chromiumos/tast/bundle/bundle.go
Project Member

Comment 16 by bugdroid1@chromium.org, Oct 4 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/6a58c6522a18dafaa9ac305c9c1970f2134915fd

commit 6a58c6522a18dafaa9ac305c9c1970f2134915fd
Author: Shuhei Takahashi <nya@chromium.org>
Date: Thu Oct 04 00:17:51 2018

tast: Update writing_tests.md to mention the errors package.

CQ-DEPEND=CL:1253682
BUG= chromium:887166 
TEST=None

Change-Id: Id2b9177635e612be881895be7cea2b75c3804611
Reviewed-on: https://chromium-review.googlesource.com/c/1257463
Reviewed-by: Dan Erat <derat@chromium.org>
Tested-by: Shuhei Takahashi <nya@chromium.org>

[modify] https://crrev.com/6a58c6522a18dafaa9ac305c9c1970f2134915fd/docs/writing_tests.md
Project Member

Comment 19 by bugdroid1@chromium.org, Oct 6 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/058dcb58a72c9753607fb7357dd40e73a74537e4

commit 058dcb58a72c9753607fb7357dd40e73a74537e4
Author: Shuhei Takahashi <nya@chromium.org>
Date: Sat Oct 06 02:37:55 2018

tast: Allow defining custom error types.

Currently, we do not have a way to define custom error types that
supports error chaining and stack traces. While there is no use
case as of today, we are afraid that lack of custom error type
support can discourage users to inspect errors in proper ways.

This CL make it possible to define custom error types by embedding
*errors.E.

BUG= chromium:887166 
TEST=fast_build.sh -T -C

Change-Id: Ia221b3bdff631f20d513ac0d190900d922055f60
Reviewed-on: https://chromium-review.googlesource.com/1258623
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>

[modify] https://crrev.com/058dcb58a72c9753607fb7357dd40e73a74537e4/docs/writing_tests.md
[modify] https://crrev.com/058dcb58a72c9753607fb7357dd40e73a74537e4/src/chromiumos/tast/errors/errors.go
[modify] https://crrev.com/058dcb58a72c9753607fb7357dd40e73a74537e4/src/chromiumos/tast/errors/errors_test.go

Comment 20 by nya@chromium.org, Oct 6

Summary: Introduce error-handling library (was: Consider using third-party library for error handling)
Project Member

Comment 21 by bugdroid1@chromium.org, Oct 6 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/2b24dcc6851b57efbe21bbeca6511898654d4327

commit 2b24dcc6851b57efbe21bbeca6511898654d4327
Author: Daniel Erat <derat@chromium.org>
Date: Sat Oct 06 15:14:41 2018

tast: Add .gitignore to exclude "/bin/".

tools/run_lint.sh runs "go install" with $GOPATH set to the
root of the repository, which results in a bin/ directory
being created.

BUG= chromium:887166 
TEST=ran "git status"

Change-Id: If15307c39feabbb5fe8ac65e9f4eb55f3b78fcd9
Reviewed-on: https://chromium-review.googlesource.com/c/1266843
Reviewed-by: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>

[add] https://crrev.com/2b24dcc6851b57efbe21bbeca6511898654d4327/.gitignore
Project Member

Comment 22 by bugdroid1@chromium.org, Oct 10 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/0035bc2cafd5bbe163f049108207390639db5b1c

commit 0035bc2cafd5bbe163f049108207390639db5b1c
Author: Shuhei Takahashi <nya@chromium.org>
Date: Wed Oct 10 16:45:02 2018

tast-tests: Rewrite tests to use new errors package (automatic).

This CL updates trivial errors.New/fmt.Errorf calls that can be
rewritten automatically with the following sed script:

s/fmt\.Errorf("%s: %v", \(.*\), err)/errors.Wrap(err, \1)/
s/fmt\.Errorf("\([^%]*\): %v", \(err\|c\.chromeErr(err)\|ctx\.Err()\|call\.Err\))/errors.Wrap(\2, "\1")/
s/fmt\.Errorf("\(.*\): %v", \(.*\), \(err\|c\.chromeErr(err)\|ctx\.Err()\|call\.Err\))/errors.Wrapf(\3, "\1", \2)/
/fmt\.Errorf.*)$/{ /fmt\.Errorf.*err/!s/fmt\.Errorf/errors.Errorf/ }

and a hacky Python script to update and sort imports.

CQ-DEPEND=CL:1253682
BUG= chromium:887166 
TEST=fast_build.sh -T -C

Change-Id: I84cc20b561962c05b289d6797045628726d3ba0d
Reviewed-on: https://chromium-review.googlesource.com/1260705
Commit-Ready: Shuhei Takahashi <nya@chromium.org>
Tested-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>

[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/arc/arc.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/minidump/minidump.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/bundles/cros/security/selinux/file_label_utils.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/arc/ui/object.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/bundles/cros/ui/chromecrash/crash.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/bundles/cros/security/selinux/processes_helper.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_launcher_app.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/chrome/proc.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/vm/util.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/arc/adb.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/chrome/metrics/histogram.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/vm/container.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/bundles/cros/network/default_profile.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/input/gen/gen_constants.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/bundles/cros/example/dbus.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/remote/bundles/cros/meta/tastrun/run.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/bundles/cros/ui/mash_login.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/input/device.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/screenshot/screenshot.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/cryptohome/cryptohome.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/dbusutil/proto.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/arc/ui/device.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/graphics/utils.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/bundles/cros/ui/session_manager_respawn.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/upstart/upstart.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/bundles/cros/platform/check_processes.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/dbusutil/signal.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/bundles/cros/vm/subtest/install_package.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/bundles/cros/vm/start_crosvm.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/vm/termina.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/graphics/deqp_parser.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/vm/concierge.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/chrome/watcher.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/chrome/util.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/dbusutil/bus.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/chrome/conn.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/shill/shill.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/bundles/cros/ui/supervised/test.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/perf/perf_test.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/chrome/chrome.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/bundles/cros/security/openfds/test.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/vm/vm.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/testexec/testexec.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/power/status.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/chrome/display/display.go
[modify] https://crrev.com/0035bc2cafd5bbe163f049108207390639db5b1c/src/chromiumos/tast/local/audio/cras.go
Project Member

Comment 23 by bugdroid1@chromium.org, Oct 10 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/f01c491a3c4c02ba6e20217a59a1f1fa1868e9bb

commit f01c491a3c4c02ba6e20217a59a1f1fa1868e9bb
Author: Shuhei Takahashi <nya@chromium.org>
Date: Wed Oct 10 20:12:38 2018

tast-tests: Enable tast-lint in pre-upload hooks.

This should be after initial conversion of errors.New/fmt.Errorf
were submitted (see CQ-DEPEND).

CQ-DEPEND=CL:1260822
CQ-DEPEND=CL:1255743
CQ-DEPEND=CL:1260705
BUG= chromium:887166 
TEST=repo upload

Change-Id: I908d503425cf1890e7ad1b3bc048bee5e57ed50b
Reviewed-on: https://chromium-review.googlesource.com/1264315
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Shuhei Takahashi <nya@chromium.org>
Reviewed-by: Shuhei Takahashi <nya@chromium.org>

[add] https://crrev.com/f01c491a3c4c02ba6e20217a59a1f1fa1868e9bb/tools/run_lint.sh
[modify] https://crrev.com/f01c491a3c4c02ba6e20217a59a1f1fa1868e9bb/PRESUBMIT.cfg

Comment 25 by nya@chromium.org, Oct 12

Status: Fixed (was: Started)
►

Sign in to add a comment