Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2437)

Issue 73790043: Fix some simplestreams tests for gccgo (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 1 month ago by wallyworld
Modified:
10 years, 1 month ago
Reviewers:
mp+210324, jameinel
Visibility:
Public.

Description

Fix some simplestreams tests for gccgo Order simplestreams fetch results so tests pass on gccgo. https://code.launchpad.net/~wallyworld/juju-core/simplestreams-ordering/+merge/210324 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -20 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M environs/imagemetadata/generate_test.go View 3 chunks +4 lines, -12 lines 0 comments Download
M environs/imagemetadata/simplestreams.go View 2 chunks +14 lines, -0 lines 2 comments Download
M environs/imagemetadata/simplestreams_test.go View 2 chunks +8 lines, -8 lines 0 comments Download
M environs/tools/simplestreams.go View 3 chunks +15 lines, -0 lines 0 comments Download

Messages

Total messages: 3
wallyworld
Please take a look.
10 years, 1 month ago (2014-03-11 00:06:30 UTC) #1
jameinel
LGTM It seems like we should actually depend on the metadata requests returning sorted data, ...
10 years, 1 month ago (2014-03-11 11:32:40 UTC) #2
wallyworld
10 years, 1 month ago (2014-03-12 00:17:11 UTC) #3
https://codereview.appspot.com/73790043/diff/1/environs/imagemetadata/simples...
File environs/imagemetadata/simplestreams.go (right):

https://codereview.appspot.com/73790043/diff/1/environs/imagemetadata/simples...
environs/imagemetadata/simplestreams.go:185: Sort(metadata)
On 2014/03/11 11:32:40, jameinel wrote:
> It seems odd that we would need to sort by default, rather than just sorting
at
> the right time in tests.
> 
> Is the sort useful in general? (Does it give us any other benefit?)
> 
> I can live with it as a massive shotgun to fix gccgo issues, but it would be
> nice to understand why it is necessary. Perhaps a comment here?

I think it's nice to have stuff sorted, makes it easier to work out things when
looking at the data, and it costs almost nothing to do it. I'll add a comment.

Also, one of the sorts in the tests in not needed if we put a sort here. So I've
removed that one.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b