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

Issue 6566050: state: added add/get deep equivalence test

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 7 months ago by niemeyer
Modified:
11 years, 7 months ago
Reviewers:
mp+126318, rog
Visibility:
Public.

Description

state: added add/get deep equivalence test The equivalence tested isn't necessarily correct, and comparing private details is discouraged in the project. The implementation might choose to cache information, or to have different logic when adding or removing, and the comparison might fail despite it being correct. That said, we've had bugs with txn-revno being incorrect before, so this testing at least ensures we're conscious about such changes. https://code.launchpad.net/~niemeyer/juju-core/add-get-equivalence/+merge/126318 (do not edit description out of merge proposal)

Patch Set 1 #

Patch Set 2 : state: added add/get deep equivalence test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+46 lines, -5 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M charm/meta.go View 1 chunk +5 lines, -5 lines 0 comments Download
M state/state_test.go View 1 chunk +39 lines, -0 lines 0 comments Download

Messages

Total messages: 3
niemeyer
Please take a look.
11 years, 7 months ago (2012-09-25 18:50:58 UTC) #1
rog
LGTM
11 years, 7 months ago (2012-09-25 18:58:12 UTC) #2
niemeyer
11 years, 7 months ago (2012-09-25 19:04:47 UTC) #3
*** Submitted:

state: added add/get deep equivalence test

The equivalence tested isn't necessarily correct, and
comparing private details is discouraged in the project.
The implementation might choose to cache information, or
to have different logic when adding or removing, and the
comparison might fail despite it being correct.
That said, we've had bugs with txn-revno being incorrect
before, so this testing at least ensures we're conscious
about such changes.

R=rog
CC=
https://codereview.appspot.com/6566050
Sign in to reply to this message.

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