Re: Ensuring tests pass on gccgo
Please don't actually slow down the time it takes to land code to trunk by
actually running the tests 2x in a row (and IME gccgo test suite is
actually more than 1x the time for the golang test suite to run).
I suppose if you can put them in parallel, but I'd really like to see it
just be a CI test.
John
=:-
On Thu, May 22, 2014 at 5:43 AM, Ian Booth ian.bo...@canonical.com wrote:
Hi folks
We are working to make all juju-core unit tests pass using gccgo. In case
you
didn't already know, there's a common issue which has caused a lot of the
failures to date. Here's a quick heads up on how to deal with it.
golang-go and gcc-go have different map implementations which results in
ordering differences, affecting things like range etc (simplistically put,
gcc-go's map ordering is random whereas currently golang-go is somewhat
deterministic). Now of course, maps are unordered but what we sometimes do
in
the code is to use a map to hold some data (maybe to eliminate duplicates)
and
then expose that data via a slice or array. If we then do a c.Assert(v1,
gc.DeepEquals, v2), it will fail on gcc-go, since the order of items in
the 2
slices is different, even though the values are the same.
In such cases, we need to ensure that we use jc.SameContents rather than
DeepEquals. By such cases, that means where we truly only care about the
contents of the data and not the order.
Here's a brief example, a snippet of code taken from a constraints test.
cons := constraints.MustParse(t.cons)
unsupported, err := validator.Validate(cons)
if t.err == {
c.Assert(err, gc.IsNil)
c.Assert(unsupported, jc.SameContents, t.unsupported)
} else {
c.Assert(err, gc.ErrorMatches, t.err)
}
Above, unsupported is a []string of attribute names. We don't care what
order
they are returned in from Validate(), just that the correct values are all
there. So instead of:
c.Assert(unsupported, jc.DeepEquals, t.unsupported)
we needed to change it to:
c.Assert(unsupported, jc.SameContents, t.unsupported)
The need to use SameContents depends on the internal implementation of the
code
under test. But as a general rule, if we use SameContents where we are
saying
that order of results is not relevant, then we can be sure tests will pass
on
gcc-go as well as golang-go.
If unsure, please test on gcc-go using something like:
go test --compiler=gccgo blah
We will soon be gating trunk landings based on *both* golang-go and gcc-go
having successful test runs and we'd like it if we didn't introduce more
test
failures to fix in the interim.
--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Ensuring tests pass on gccgo
On Wed, May 21, 2014 at 10:43 PM, Ian Booth ian.bo...@canonical.com wrote:
We are working to make all juju-core unit tests pass using gccgo. In case you
didn't already know, there's a common issue which has caused a lot of the
failures to date. Here's a quick heads up on how to deal with it.
golang-go and gcc-go have different map implementations which results in
ordering differences, affecting things like range etc (simplistically put,
gcc-go's map ordering is random whereas currently golang-go is somewhat
deterministic).
This is changing in the main compiler as well, in Go 1.3:
http://tip.golang.org/doc/go1.3#map
So it'll become even less deterministic there as well.
Now of course, maps are unordered but what we sometimes do in
the code is to use a map to hold some data (maybe to eliminate duplicates) and
then expose that data via a slice or array. If we then do a c.Assert(v1,
gc.DeepEquals, v2), it will fail on gcc-go, since the order of items in the 2
slices is different, even though the values are the same.
If that's really the case, it's definitely a bug in gccgo. gocheck's
DeepEquals is implemented in terms of reflect.DeepEqual, which should
not care about the map order. In the standard library of the main
compiler, it clearly does not:
for _, k := range v1.MapKeys() {
if !deepValueEqual(v1.MapIndex(k), v2.MapIndex(k),
visited, depth+1) {
return false
}
}
So gocheck's DeepEquals is fine for such map tests, assuming no bugs
in the underlying implementation.
gustavo @ http://niemeyer.net
--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
On 14 May 2014 10:24, Tim Penhey tim.pen...@canonical.com wrote: Hi all, I took it upon myself to get Rog's errgo library used inside juju-core. Thanks for doing this. Dimiter recently did a hunk of work in the juju-core/errors package to have functions to add context to some core error types while keeping their type. What I did was to move this errors package out of juju-core and into its own package, and update it to use the errgo as a base. For those that are interested the diff for the errors_test (to show that everything works as before) is as shown at the bottom of this email. I also added in simple methods based on my earlier error handling package, but changed them to be based on errgo. This adds in the functions: New, Errorf, Check, Trace, Annotate, Annotatef, Wrap, ErrorStack. Any particular reason to wrap the errgo functions rather than using errgo directly? Personally, I see the role of the errors package to be about specific error classifications (something that errgo tries hard to be entirely agnostic about), but errgo could, and I believe should, be used directly for all the more general calls. Since it's likely that other packages will start to use errgo, and they don't necessarily want to be importing juju/errors, there are advantages to using the same error generation and classification style that any other errgo-using package would use - in particular, it makes the code easier to manipulate programmatically. There are also advantages in not having two subtly different APIs around, even if one is implemented in terms of the other. I believe we should pick one set of calls and use them consistently throughout the code base. BTW, I have a gofix clone which automates 95+% of the changes to use errgo - it's at code.google.com/p/rog-go/exp/cmd/errfix It splits the changes into several modules so that the changes can be made in more-easily verifiable steps. I have tested juju/errors with both gc and gccgo and the tests pass with both. Interestingly juju/errgo has a few test failures with gccgo, but Dave is looking at them and assures me they are simple fixes. As an aside, I do think that any new dependencies we add should pass all tests with both gc and gccgo. Thanks a lot for doing this. I really should install and try gccgo! cheers, rog. -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Ensuring tests pass on gccgo
Although using DeepEquals on sorted lists does make for
easier-to-understand test failure messages, the burden on the developer to
sort the slices when they write the test doesn't seem worth it to me. We
write tests a lot more often than we need to debug failing tests. If it
was just slices of strings, then sorting is easy enough, but for other
random types, you now have to figure out how to sort the types in a fully
deterministic way, which can be difficult and error prone for large
structs, especially as those structs change.
It's much easier and less error prone to say just use jc.SameContents.
On May 22, 2014 10:42 AM, Gustavo Niemeyer gustavo.nieme...@canonical.com
wrote:
I apologize. After reading one of the review entries using
SameContents, I see I've misunderstood what the problem is about. The
v1 and v2 in your example is a slice, not a map, which means the test
is comparing slices of undefined order. This is certainly bogus and
needs addressing in the code itself, irrespective of any map-related
issues.
What I generally do in those cases is to define the ordering of the
slice. Besides enabling DeepEquals, it also makes the visual
comparison more pleasant.
On Thu, May 22, 2014 at 11:34 AM, Gustavo Niemeyer
gustavo.nieme...@canonical.com wrote:
On Wed, May 21, 2014 at 10:43 PM, Ian Booth ian.bo...@canonical.com
wrote:
We are working to make all juju-core unit tests pass using gccgo. In
case you
didn't already know, there's a common issue which has caused a lot of
the
failures to date. Here's a quick heads up on how to deal with it.
golang-go and gcc-go have different map implementations which results in
ordering differences, affecting things like range etc (simplistically
put,
gcc-go's map ordering is random whereas currently golang-go is somewhat
deterministic).
This is changing in the main compiler as well, in Go 1.3:
http://tip.golang.org/doc/go1.3#map
So it'll become even less deterministic there as well.
Now of course, maps are unordered but what we sometimes do in
the code is to use a map to hold some data (maybe to eliminate
duplicates) and
then expose that data via a slice or array. If we then do a c.Assert(v1,
gc.DeepEquals, v2), it will fail on gcc-go, since the order of items in
the 2
slices is different, even though the values are the same.
If that's really the case, it's definitely a bug in gccgo. gocheck's
DeepEquals is implemented in terms of reflect.DeepEqual, which should
not care about the map order. In the standard library of the main
compiler, it clearly does not:
for _, k := range v1.MapKeys() {
if !deepValueEqual(v1.MapIndex(k), v2.MapIndex(k),
visited, depth+1) {
return false
}
}
So gocheck's DeepEquals is fine for such map tests, assuming no bugs
in the underlying implementation.
gustavo @ http://niemeyer.net
--
gustavo @ http://niemeyer.net
--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Proposed new dependency: github.com/juju/errors (and github.com/juju/errgo)
I agree with Roger, I don't think a wrapper around Errgo is the right way to go. If we don't like the way errgo behaves, we should change it. It's our package, let's have it work the way we want it to work. On Thu, May 22, 2014 at 10:47 AM, roger peppe rogpe...@gmail.com wrote: On 14 May 2014 10:24, Tim Penhey tim.pen...@canonical.com wrote: Hi all, I took it upon myself to get Rog's errgo library used inside juju-core. Thanks for doing this. Dimiter recently did a hunk of work in the juju-core/errors package to have functions to add context to some core error types while keeping their type. What I did was to move this errors package out of juju-core and into its own package, and update it to use the errgo as a base. For those that are interested the diff for the errors_test (to show that everything works as before) is as shown at the bottom of this email. I also added in simple methods based on my earlier error handling package, but changed them to be based on errgo. This adds in the functions: New, Errorf, Check, Trace, Annotate, Annotatef, Wrap, ErrorStack. Any particular reason to wrap the errgo functions rather than using errgo directly? Personally, I see the role of the errors package to be about specific error classifications (something that errgo tries hard to be entirely agnostic about), but errgo could, and I believe should, be used directly for all the more general calls. Since it's likely that other packages will start to use errgo, and they don't necessarily want to be importing juju/errors, there are advantages to using the same error generation and classification style that any other errgo-using package would use - in particular, it makes the code easier to manipulate programmatically. There are also advantages in not having two subtly different APIs around, even if one is implemented in terms of the other. I believe we should pick one set of calls and use them consistently throughout the code base. BTW, I have a gofix clone which automates 95+% of the changes to use errgo - it's at code.google.com/p/rog-go/exp/cmd/errfix It splits the changes into several modules so that the changes can be made in more-easily verifiable steps. I have tested juju/errors with both gc and gccgo and the tests pass with both. Interestingly juju/errgo has a few test failures with gccgo, but Dave is looking at them and assures me they are simple fixes. As an aside, I do think that any new dependencies we add should pass all tests with both gc and gccgo. Thanks a lot for doing this. I really should install and try gccgo! cheers, rog. -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
