style: EXPECT_EQ(expected, actual) vs. (actual, expected) ?

4,776 views
Skip to first unread message

James Cook

unread,
May 9, 2018, 4:48:03 PM5/9/18
to chromium-dev
Our historical version of GTest used (expected, actual) and some reviewers prefer new tests use that order.

However, our current GTest doesn't care about the order:

EXPECT_EQ doesn't care about the order either. And we have new tests written both ways.

I prefer to write expectations similar to an if() statement, which I would write as:
    if (DoFoo() == 123)

and not
    if (123 == DoFoo())

so I end up with:
    EXPECT_EQ(DoFoo(), 123)

Is that acceptable, oh style arbiters?

James


Daniel Cheng

unread,
May 9, 2018, 4:50:44 PM5/9/18
to James Cook, Chromium-dev
​I think it's fine to use either order, since googletest explicitly gave up on this… because people were already inconsistent about following the convention. I'd say be consistent within a test file though.

Daniel​

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKLzqjGo4kN7SBP1pd_-6W9VReedbNqGQR%2B8gi6SsPUYYZVUOg%40mail.gmail.com.

Jay Civelli

unread,
May 9, 2018, 4:52:06 PM5/9/18
to jame...@chromium.org, chromium-dev
In Java, JUnit puts it (expected, actual). Since the chromium Java tests do make use of it in that order I have personally used that order as well with GTest.


On Wed, May 9, 2018 at 1:47 PM James Cook <jame...@chromium.org> wrote:
--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

Lei Zhang

unread,
May 9, 2018, 4:56:36 PM5/9/18
to James Cook, chromium-dev
I used to care a lot more with older GTest, because it used to be the
case that if one write the arguments in the wrong order:
EXPECT_EQ(actual, expected)

then on failure GTest will print out:

Expected: actual
Actual: expected

and that was super confusing. Newer Gtest will print out:

Expected equality of these values:
var_name1
Which is: var_name1_value
var_name2
Which is: var_name2_value

So the ordering does not matter. I tend to still write
EXPECT_EQ(expected, actual) out of habit.
> --
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev
> ---
> You received this message because you are subscribed to the Google Groups
> "Chromium-dev" group.

Peter Kasting

unread,
May 9, 2018, 4:56:51 PM5/9/18
to Daniel Cheng, James Cook, Chromium-dev
On Wed, May 9, 2018 at 1:49 PM Daniel Cheng <dch...@chromium.org> wrote:
​I think it's fine to use either order, since googletest explicitly gave up on this… because people were already inconsistent about following the convention. I'd say be consistent within a test file though.

+1

PK 

Harald Alvestrand

unread,
May 10, 2018, 1:42:55 AM5/10/18
to Peter Kasting, Daniel Cheng, James Cook, Chromium-dev
The big gotcha here is EXPECT_LT and friends. They work opposite in gtest and testharness.js, and have bitten me repeatedly.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFBEj723ySJPLgRR4mzoBz80zBg7N5LeJoCM3T1XXrfTNA%40mail.gmail.com.

Harald Alvestrand

unread,
May 14, 2018, 3:44:11 AM5/14/18
to Philip Jägenstedt, Peter Kasting, Daniel Cheng, James Cook, Chromium-dev
Order-agnosticism, and as a side effect banning assert_lt and friends would work for me.

I note that node.js' builtin "assert" library does assert.deepStrictEqual(actual, expected), so there is probably no hope for consistency between libraries.



On Thu, May 10, 2018 at 12:43 PM, Philip Jägenstedt <foo...@google.com> wrote:
Yep. Additionally, people familiar with gtest will often use assert_equals(expected, actual) and similar in testharness.js tests where it doesn't cause the test to fail but does make the error message confusing.

Maybe the fix is to make testharness.js order-agnostic too, but of course that doesn't work for < or >.

On Thu, May 10, 2018, 07:42 'Harald Alvestrand' via Chromium-dev <chromi...@chromium.org> wrote:
The big gotcha here is EXPECT_LT and friends. They work opposite in gtest and testharness.js, and have bitten me repeatedly.

Den ons. 9. mai 2018, 22:56 skrev Peter Kasting <pkas...@chromium.org>:
On Wed, May 9, 2018 at 1:49 PM Daniel Cheng <dch...@chromium.org> wrote:
​I think it's fine to use either order, since googletest explicitly gave up on this… because people were already inconsistent about following the convention. I'd say be consistent within a test file though.

+1

PK 

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Michael Giuffrida

unread,
May 14, 2018, 4:00:23 AM5/14/18
to h...@google.com, Philip Jägenstedt, Peter Kasting, Daniel Cheng, James Cook, Chromium-dev
chai.js also uses the assert.equal(actual, expected) order, which is the JS library we use in Chromium for JavaScript browser tests (especially for MD WebUI).

To add to the confusion, we mostly use this library through our own JS interface which reverses the argument order to conform to assertEquals(expected, actual) ordering.


On Mon, May 14, 2018 at 12:43 AM 'Harald Alvestrand' via Chromium-dev <chromi...@chromium.org> wrote:
Order-agnosticism, and as a side effect banning assert_lt and friends would work for me.

I note that node.js' builtin "assert" library does assert.deepStrictEqual(actual, expected), so there is probably no hope for consistency between libraries.



On Thu, May 10, 2018 at 12:43 PM, Philip Jägenstedt <foo...@google.com> wrote:
Yep. Additionally, people familiar with gtest will often use assert_equals(expected, actual) and similar in testharness.js tests where it doesn't cause the test to fail but does make the error message confusing.

Maybe the fix is to make testharness.js order-agnostic too, but of course that doesn't work for < or >.
On Thu, May 10, 2018, 07:42 'Harald Alvestrand' via Chromium-dev <chromi...@chromium.org> wrote:
The big gotcha here is EXPECT_LT and friends. They work opposite in gtest and testharness.js, and have bitten me repeatedly.

Den ons. 9. mai 2018, 22:56 skrev Peter Kasting <pkas...@chromium.org>:
On Wed, May 9, 2018 at 1:49 PM Daniel Cheng <dch...@chromium.org> wrote:
​I think it's fine to use either order, since googletest explicitly gave up on this… because people were already inconsistent about following the convention. I'd say be consistent within a test file though.

+1

PK 

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

Nico Weber

unread,
May 14, 2018, 8:02:15 AM5/14/18
to Harald Alvestrand, Philip Jägenstedt, Peter Kasting, Daniel Cheng, James Cook, Chromium-dev
On Mon, May 14, 2018 at 3:42 AM, 'Harald Alvestrand' via Chromium-dev <chromi...@chromium.org> wrote:
Order-agnosticism, and as a side effect banning assert_lt and friends would work for me.

(from what I understand, nothing on this thread suggested banning assert_lt. As I understand it, the thead is about _EQ, and since == is symmetric it in a way makes sense to make _EQ symmetric too. < isn't symmetric, so that argument doesn't apply there. It's unfortunate that assert_lt(a, b) in one of our js testing libraries apparently means "check that b < a", that does seem pretty surprising.)
 
Reply all
Reply to author
Forward
0 new messages