PaymentApp: Fix params order of EXPECT/ASSERTs in unit tests.

PaymentApp: Fix params order of EXPECT/ASSERTs in unit tests.

The correct order should be (expected, actual) as per macro definitions in
gtest.h.

BUG=661608
Committed: https://crrev.com/d68e475ced5992041ee85217ec6b95cfe744f254
Cr-Commit-Position: refs/heads/master@{#439862}

[zino, 2016-12-19 17:52:45]

Description was changed from

==========
Fix params order of EXPECT/ASSERTs in content/browser/payments/*unittest.cc.

The correct order should be (expected, actual) as per macro definitions in
gtest.h.

BUG=661608
==========

to

==========
PaymentApp: Fix params order of EXPECT/ASSERTs in unit tests.

The correct order should be (expected, actual) as per macro definitions in
gtest.h.

BUG=661608
==========

[zino, 2016-12-19 17:55:53]

jinho.bang@samsung.com changed reviewers:
+ rouslan@chromium.org

[zino, 2016-12-19 17:57:31]

PTAL

I mis-used some EXPECT/ASSERTs..

[please use gerrit instead, 2016-12-19 20:27:58]

https://codereview.chromium.org/2585303002/diff/1/content/browser/payments/pa...
File content/browser/payments/payment_app_context_impl_unittest.cc (right):

https://codereview.chromium.org/2585303002/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_context_impl_unittest.cc:103:
EXPECT_EQ("payment-app-icon", manifest.second->icon.value());
If you construct an expected manifest, can you compare the two structs instead
of field-by-field?

https://codereview.chromium.org/2585303002/diff/1/content/browser/payments/pa...
File content/browser/payments/payment_app_manager_unittest.cc (right):

https://codereview.chromium.org/2585303002/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_manager_unittest.cc:141:
EXPECT_EQ(payments::mojom::PaymentAppManifestError::
Maybe you should put "using payments::mojom::PaymentAppManifestError" at the top
to avoid typing "payments::mojom::" so many times.

[zino, 2016-12-20 17:11:28]

Thank you for review.

https://codereview.chromium.org/2585303002/diff/1/content/browser/payments/pa...
File content/browser/payments/payment_app_context_impl_unittest.cc (right):

https://codereview.chromium.org/2585303002/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_context_impl_unittest.cc:103:
EXPECT_EQ("payment-app-icon", manifest.second->icon.value());
On 2016/12/19 20:27:58, rouslan-intermittent-holidays wrote:
> If you construct an expected manifest, can you compare the two structs instead
> of field-by-field?

Done.

I agree with you. It makes the codes simpler.
But it's not easy to read its result. Which do you prefer to?

../../content/browser/payments/payment_app_manager_unittest.cc:96: Failure
Value of: read_manifest
  Actual: 8-byte object <C0-D6 05-1D F8-7F 00-00>
Expected: expected
Which is: 8-byte object <80-D7 05-1D F8-7F 00-00>
[  FAILED  ] PaymentAppManagerTest.SetAndGetManifest (4 ms)

https://codereview.chromium.org/2585303002/diff/1/content/browser/payments/pa...
File content/browser/payments/payment_app_manager_unittest.cc (right):

https://codereview.chromium.org/2585303002/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_manager_unittest.cc:141:
EXPECT_EQ(payments::mojom::PaymentAppManifestError::
On 2016/12/19 20:27:58, rouslan-intermittent-holidays wrote:
> Maybe you should put "using payments::mojom::PaymentAppManifestError" at the
top
> to avoid typing "payments::mojom::" so many times.

Done.

[please use gerrit instead, 2016-12-20 17:23:37]

lgtm % comments

https://codereview.chromium.org/2585303002/diff/1/content/browser/payments/pa...
File content/browser/payments/payment_app_context_impl_unittest.cc (right):

https://codereview.chromium.org/2585303002/diff/1/content/browser/payments/pa...
content/browser/payments/payment_app_context_impl_unittest.cc:103:
EXPECT_EQ("payment-app-icon", manifest.second->icon.value());
On 2016/12/20 17:11:28, zino wrote:
> On 2016/12/19 20:27:58, rouslan-intermittent-holidays wrote:
> > If you construct an expected manifest, can you compare the two structs
instead
> > of field-by-field?
> 
> Done.
> 
> I agree with you. It makes the codes simpler.
> But it's not easy to read its result. Which do you prefer to?
> 
> ../../content/browser/payments/payment_app_manager_unittest.cc:96: Failure
> Value of: read_manifest
>   Actual: 8-byte object <C0-D6 05-1D F8-7F 00-00>
> Expected: expected
> Which is: 8-byte object <80-D7 05-1D F8-7F 00-00>
> [  FAILED  ] PaymentAppManagerTest.SetAndGetManifest (4 ms)

You're right, it's better to keep error messages clear. You can either go back
to your previous approach or implement this function:

std::ostream& operator<<(std::ostream& out, const PutYourTypeNameHere&
object_name);

[zino, 2016-12-20 18:33:32]

On 2016/12/20 17:23:37, rouslan-intermittent-holidays wrote:
> lgtm % comments
> 
>
https://codereview.chromium.org/2585303002/diff/1/content/browser/payments/pa...
> File content/browser/payments/payment_app_context_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2585303002/diff/1/content/browser/payments/pa...
> content/browser/payments/payment_app_context_impl_unittest.cc:103:
> EXPECT_EQ("payment-app-icon", manifest.second->icon.value());
> On 2016/12/20 17:11:28, zino wrote:
> > On 2016/12/19 20:27:58, rouslan-intermittent-holidays wrote:
> > > If you construct an expected manifest, can you compare the two structs
> instead
> > > of field-by-field?
> > 
> > Done.
> > 
> > I agree with you. It makes the codes simpler.
> > But it's not easy to read its result. Which do you prefer to?
> > 
> > ../../content/browser/payments/payment_app_manager_unittest.cc:96: Failure
> > Value of: read_manifest
> >   Actual: 8-byte object <C0-D6 05-1D F8-7F 00-00>
> > Expected: expected
> > Which is: 8-byte object <80-D7 05-1D F8-7F 00-00>
> > [  FAILED  ] PaymentAppManagerTest.SetAndGetManifest (4 ms)
> 
> You're right, it's better to keep error messages clear. You can either go back
> to your previous approach or implement this function:
> 
> std::ostream& operator<<(std::ostream& out, const PutYourTypeNameHere&
> object_name);

Thank you for guide. I tried to implement it at the moment but it's not simple
than my thought.
So, I'll try it again in follow-up CL. I just came back to the previous
approach.

Thanks.

[zino, 2016-12-20 18:36:56]

The CQ bit was checked by jinho.bang@samsung.com

[zino, 2016-12-20 18:36:56]

The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2585303002/#ps40001
(title: "PaymentApp: Fix params order of EXPECT/ASSERTs in unit tests.")

[commit-bot: I haz the power, 2016-12-20 18:37:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 19:08:14]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482259016511820,
"parent_rev": "13e29380c873185b569151d76e45f5d97128e3d5", "commit_rev":
"8f9faa85ed92a0931555e353a391fa567eb2b260"}

[commit-bot: I haz the power, 2016-12-20 19:08:43]

Description was changed from

==========
PaymentApp: Fix params order of EXPECT/ASSERTs in unit tests.

The correct order should be (expected, actual) as per macro definitions in
gtest.h.

BUG=661608
==========

to

==========
PaymentApp: Fix params order of EXPECT/ASSERTs in unit tests.

The correct order should be (expected, actual) as per macro definitions in
gtest.h.

BUG=661608

Review-Url: https://codereview.chromium.org/2585303002
==========

[commit-bot: I haz the power, 2016-12-20 19:08:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-20 19:11:43]

Description was changed from

==========
PaymentApp: Fix params order of EXPECT/ASSERTs in unit tests.

The correct order should be (expected, actual) as per macro definitions in
gtest.h.

BUG=661608

Review-Url: https://codereview.chromium.org/2585303002
==========

to

==========
PaymentApp: Fix params order of EXPECT/ASSERTs in unit tests.

The correct order should be (expected, actual) as per macro definitions in
gtest.h.

BUG=661608

Committed: https://crrev.com/d68e475ced5992041ee85217ec6b95cfe744f254
Cr-Commit-Position: refs/heads/master@{#439862}
==========

[commit-bot: I haz the power, 2016-12-20 19:11:43]

Patchset 3 (id:??) landed as
https://crrev.com/d68e475ced5992041ee85217ec6b95cfe744f254
Cr-Commit-Position: refs/heads/master@{#439862}