Add CHECK_IS_TEST [chromium/src : main]

0 views
Skip to first unread message

Roland Bock (Gerrit)

unread,
Jun 27, 2022, 9:16:32 AMJun 27
to browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Daniel Cheng, danakj, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Daniel Cheng, danakj.

View Change

1 comment:

  • Patchset:

    • Patch Set #17:

      Hi,

      This is the implementation of `CHECK_IS_TEST` as discussed in go/is-this-a-test.
      Please take a look.

      Thanks,
      Roland

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 17
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
Gerrit-Attention: danakj <dan...@chromium.org>
Gerrit-Comment-Date: Mon, 27 Jun 2022 13:16:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Daniel Cheng (Gerrit)

unread,
Jun 28, 2022, 1:07:31 PMJun 28
to Roland Bock, browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Daniel Cheng, danakj, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Roland Bock, danakj.

View Change

3 comments:

  • File base/check_is_test.h:

    • Patch Set #17, Line 17:

      , in which the cost of avoiding
      // special test situations was deemed too high

      I'm not convinced this is true and would prefer to remove this statement.

    • Patch Set #17, Line 43: #define CHECK_IS_TEST() base::internal::check_is_test_impl();

      Can we move the macros up above the base::internal declarations? Also, when would one want to use DCHECK_IS_TEST()? Do we have this because of anticipated need or potential need?

  • File base/test/test_suite.cc:

    • Patch Set #17, Line 353: }();

      Hrm.

      I would prefer that this be explicitly called from test main. Is that problematic/hard-to-do/requires changing too many things? For example, would it be possible to have this in unit_test_launcher.cc?

      The reason for this is is two-fold:
      1. less clever but more straightforward (even if potentially more verbose) init
      2. not requiring the use of std::atomic<bool> and letting TSAN complain if we are initialising this racily somehow

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 17
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: danakj <dan...@chromium.org>
Gerrit-Attention: Roland Bock <rb...@google.com>
Gerrit-Comment-Date: Tue, 28 Jun 2022 17:07:22 +0000

Roland Bock (Gerrit)

unread,
Jun 30, 2022, 11:10:21 AMJun 30
to browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Daniel Cheng, danakj, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Daniel Cheng, danakj.

View Change

3 comments:

  • File base/check_is_test.h:

    • Patch Set #17, Line 17:

      , in which the cost of avoiding
      // special test situations was deemed too high

      I'm not convinced this is true and would prefer to remove this statement.

    • Good point, speculation on my end. Rephrased.

    • Can we move the macros up above the base::internal declarations? Also, when would one want to use DC […]

      Moved the code.

      Re DCHECK: See discussions in the doc. Several folks expressed fear that a CHECK might be too disruptive. Personally, I disagree, but if it helps to get people started to instrument test-only paths, I am willing to accept it.

      I updated the comment above and the documentation.

  • File base/test/test_suite.cc:

    • Hrm. […]

      Thanks for the pointer. I moved the call to

      base/test/launcher/unit_test_launcher.cc,
      base/test/launcher/unit_test_launcher_ios.cc and
      chrome/test/base/chrome_test_launcher.cc

      This covers both unit and browser tests (see https://chromium-review.googlesource.com/c/chromium/src/+/3737804).

      And yes, it is much nicer to not user extra clever code :-)

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 19
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
Gerrit-Attention: danakj <dan...@chromium.org>
Gerrit-Comment-Date: Thu, 30 Jun 2022 15:10:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Daniel Cheng <dch...@chromium.org>
Gerrit-MessageType: comment

Daniel Cheng (Gerrit)

unread,
Jun 30, 2022, 3:20:26 PMJun 30
to Roland Bock, browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Daniel Cheng, danakj, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Roland Bock, danakj.

View Change

5 comments:

  • File base/check_is_test.h:

  • File base/check_is_test.cc:

    • Patch Set #19, Line 12: std::atomic<bool> g_this_is_a_test = false;

      Now that we don't have the static initializer business, is it possible to just use a regular bool here?

    • Patch Set #19, Line 30: BASE_EXPORT

      BASE_EXPORT shouldn't be needed here; it should just be needed on the declaration (then we can drop lines 28-29)

  • File base/test/allow_check_is_test_to_be_called.h:

    • Patch Set #19, Line 9: // This is called exactly once in `base::TestSuite`.

      Very minor nit: newline before to balance it out

  • File styleguide/c++/c++.md:

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 19
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: danakj <dan...@chromium.org>
Gerrit-Attention: Roland Bock <rb...@google.com>
Gerrit-Comment-Date: Thu, 30 Jun 2022 19:20:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Roland Bock (Gerrit)

unread,
Jul 1, 2022, 5:22:01 AMJul 1
to browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Daniel Cheng, danakj, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Daniel Cheng, danakj.

View Change

8 comments:

  • File base/check_is_test.h:

    • Done

  • File base/check_is_test.h:

    • Good point, speculation on my end. Rephrased.

      Done

    • Moved the code. […]

      Done

  • File base/check_is_test.cc:

    • Now that we don't have the static initializer business, is it possible to just use a regular bool he […]

      Yes, I think that's safe now.

    • BASE_EXPORT shouldn't be needed here; it should just be needed on the declaration (then we can drop […]

      `AllowCheckIsTestToBeCalled` is declared in base/test/allow_check_is_test_to_be_called.h which can be only included in test code.
      I therefore cannot include it here, AFAICT.

      Updated the comment.

  • File base/test/allow_check_is_test_to_be_called.h:

    • Patch Set #19, Line 9: // This is called exactly once in `base::TestSuite`.

      Very minor nit: newline before to balance it out

    • Done.

      Oh, and it also is not called in base::TestSuite anymore. Made the statement more abstract.

  • File base/test/test_suite.cc:

    • Thanks for the pointer. I moved the call to […]

      Done

  • File styleguide/c++/c++.md:

    • "Strongly prefer CHECK_IS_TEST() whenever possible. […]

      Thanks for the suggestion!

      Added it to the header, too.

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 21
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: Daniel Cheng <dch...@chromium.org>
Gerrit-Attention: danakj <dan...@chromium.org>
Gerrit-Comment-Date: Fri, 01 Jul 2022 09:21:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Daniel Cheng <dch...@chromium.org>
Comment-In-Reply-To: Roland Bock <rb...@google.com>
Gerrit-MessageType: comment

Daniel Cheng (Gerrit)

unread,
Jul 1, 2022, 1:39:57 PMJul 1
to Roland Bock, browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Daniel Cheng, danakj, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Roland Bock, danakj.

Patch set 21:Code-Review +1

View Change

2 comments:

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 21
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: danakj <dan...@chromium.org>
Gerrit-Attention: Roland Bock <rb...@google.com>
Gerrit-Comment-Date: Fri, 01 Jul 2022 17:39:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Roland Bock (Gerrit)

unread,
Jul 2, 2022, 3:11:46 AMJul 2
to browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Daniel Cheng, danakj, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: danakj.

View Change

1 comment:

  • File base/check_is_test.cc:

    • Done

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 22
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: danakj <dan...@chromium.org>
Gerrit-Comment-Date: Sat, 02 Jul 2022 07:11:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Daniel Cheng <dch...@chromium.org>
Gerrit-MessageType: comment

Roland Bock (Gerrit)

unread,
Jul 7, 2022, 12:03:27 PMJul 7
to browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Marc Treib, Daniel Cheng, danakj, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Marc Treib, danakj.

View Change

1 comment:

  • Patchset:

    • Patch Set #22:

      @Marc: This is based on go/is-this-a-test. Please take a look at chrome_test_launcher.cc.

      @Dana: Friendly ping :-)

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 22
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: danakj <dan...@chromium.org>
Gerrit-Attention: Marc Treib <tr...@chromium.org>
Gerrit-Comment-Date: Thu, 07 Jul 2022 16:03:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

danakj (Gerrit)

unread,
Jul 7, 2022, 12:05:14 PMJul 7
to Roland Bock, browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Marc Treib, Daniel Cheng, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Marc Treib, Roland Bock.

View Change

1 comment:

  • File styleguide/c++/c++.md:

    • Patch Set #22, Line 355: run in tests. `DCHECK_IS_TEST();` can be used in case the code owners

      Given this is a new thing, and we can just revert/remove the CHECK or fix it if we're wrong, why do we want to support the DCHECK mode at all?

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 22
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: Marc Treib <tr...@chromium.org>
Gerrit-Attention: Roland Bock <rb...@google.com>
Gerrit-Comment-Date: Thu, 07 Jul 2022 16:04:59 +0000

Roland Bock (Gerrit)

unread,
Jul 7, 2022, 1:51:23 PMJul 7
to browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Marc Treib, Daniel Cheng, danakj, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Marc Treib, danakj.

View Change

1 comment:

  • File styleguide/c++/c++.md:

    • Given this is a new thing, and we can just revert/remove the CHECK or fix it if we're wrong, why do […]

      I would prefer to only have the CHECK_IS_TEST, but there were concerns during the review of go/is-this-a-test that this cause crashes on the stable channel.

      Given such concerns, I'd rather have test-code instrumented with DCHECK_IS_TEST than leaving it uninstrumented.

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 22
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: danakj <dan...@chromium.org>
Gerrit-Attention: Marc Treib <tr...@chromium.org>
Gerrit-Comment-Date: Thu, 07 Jul 2022 17:51:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: danakj <dan...@chromium.org>
Gerrit-MessageType: comment

danakj (Gerrit)

unread,
Jul 7, 2022, 1:54:29 PMJul 7
to Roland Bock, browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Marc Treib, Daniel Cheng, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Marc Treib, Roland Bock.

View Change

1 comment:

  • File styleguide/c++/c++.md:

    • I would prefer to only have the CHECK_IS_TEST, but there were concerns during the review of go/is-th […]

      Hm, the conclusion feels a bit contradictory.

      1. Starting with DCHECK_IS_TEST to prevent crashes if we do a large change?
      2. Advice in here to not use DCHECK_IS_TEST.
      3. No intention to make a large change that would require DCHECK_IS_TEST

      Where do we land there?

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 22
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: Marc Treib <tr...@chromium.org>
Gerrit-Attention: Roland Bock <rb...@google.com>
Gerrit-Comment-Date: Thu, 07 Jul 2022 17:54:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: danakj <dan...@chromium.org>

danakj (Gerrit)

unread,
Jul 7, 2022, 1:55:38 PMJul 7
to Roland Bock, browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Marc Treib, Daniel Cheng, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Marc Treib, Roland Bock.

View Change

1 comment:

  • File styleguide/c++/c++.md:

    • Hm, the conclusion feels a bit contradictory. […]

      Additionally - given DCHECK_IS_TEST would basically only do something in tests, what is the point?

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 22
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: Marc Treib <tr...@chromium.org>
Gerrit-Attention: Roland Bock <rb...@google.com>
Gerrit-Comment-Date: Thu, 07 Jul 2022 17:55:31 +0000

Roland Bock (Gerrit)

unread,
Jul 7, 2022, 2:22:10 PMJul 7
to browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Marc Treib, Daniel Cheng, danakj, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Marc Treib, danakj.

View Change

1 comment:

  • File styleguide/c++/c++.md:

    • Additionally - given DCHECK_IS_TEST would basically only do something in tests, what is the point?

      Don't we ship a small percentage of images with DCHECK on? I am pretty sure I read this a while ago.
      If that is fake news then DCHECK_IS_TEST would be useless indeed.

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 22
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: danakj <dan...@chromium.org>
Gerrit-Attention: Marc Treib <tr...@chromium.org>
Gerrit-Comment-Date: Thu, 07 Jul 2022 18:21:53 +0000

danakj (Gerrit)

unread,
Jul 7, 2022, 2:37:50 PMJul 7
to Roland Bock, browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Marc Treib, Daniel Cheng, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Marc Treib, Roland Bock.

View Change

1 comment:

  • File styleguide/c++/c++.md:

    • Don't we ship a small percentage of images with DCHECK on? I am pretty sure I read this a while ago. […]

      They report the DCHECKs as crash reports but they don't actually crash.

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 22
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: Marc Treib <tr...@chromium.org>
Gerrit-Attention: Roland Bock <rb...@google.com>
Gerrit-Comment-Date: Thu, 07 Jul 2022 18:37:36 +0000

Roland Bock (Gerrit)

unread,
Jul 7, 2022, 2:50:15 PMJul 7
to browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Marc Treib, Daniel Cheng, danakj, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Marc Treib, danakj.

View Change

1 comment:

  • File styleguide/c++/c++.md:

    • They report the DCHECKs as crash reports but they don't actually crash.

      Got it. So it would not be completely useless...

      That being said: We could just drop DCHECK_IS_TEST from this CL and start with the CHECK_IS_TEST only. It would send a clearer message and would not prevent us from adding it later, if someone presented a compelling usecase/example.

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 22
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: danakj <dan...@chromium.org>
Gerrit-Attention: Marc Treib <tr...@chromium.org>
Gerrit-Comment-Date: Thu, 07 Jul 2022 18:50:01 +0000

danakj (Gerrit)

unread,
Jul 7, 2022, 2:59:28 PMJul 7
to Roland Bock, browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Marc Treib, Daniel Cheng, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Marc Treib, Roland Bock.

View Change

1 comment:

  • File styleguide/c++/c++.md:

    • Got it. So it would not be completely useless... […]

      It would not be completely but I don't see when it should ever be used. So yes let's drop it for now is my preference.

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 22
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: Marc Treib <tr...@chromium.org>
Gerrit-Attention: Roland Bock <rb...@google.com>
Gerrit-Comment-Date: Thu, 07 Jul 2022 18:59:17 +0000

Marc Treib (Gerrit)

unread,
Jul 8, 2022, 3:21:50 AMJul 8
to Roland Bock, browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Marc Treib, Daniel Cheng, danakj, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Roland Bock.

Patch set 22:Code-Review +1

View Change

1 comment:

  • Patchset:

    • Patch Set #22:

      @Marc: This is based on go/is-this-a-test. Please take a look at chrome_test_launcher.cc.

    • LGTM for this file. I've only skimmed the rest of the CL, and haven't given any thought to the CHECK vs DCHECK discussion.

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 22
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: Roland Bock <rb...@google.com>
Gerrit-Comment-Date: Fri, 08 Jul 2022 07:21:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes

Roland Bock (Gerrit)

unread,
Jul 8, 2022, 7:18:30 AMJul 8
to browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Marc Treib, Daniel Cheng, danakj, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: danakj.

View Change

1 comment:

  • File styleguide/c++/c++.md:

    • It would not be completely but I don't see when it should ever be used. […]

      Dropped. Thanks for the discussion!

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 23
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: danakj <dan...@chromium.org>
Gerrit-Comment-Date: Fri, 08 Jul 2022 11:18:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: danakj <dan...@chromium.org>

danakj (Gerrit)

unread,
Jul 12, 2022, 12:38:17 PMJul 12
to Roland Bock, browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Marc Treib, Daniel Cheng, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Roland Bock.

Patch set 23:Code-Review +1

View Change

2 comments:

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 23
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: Roland Bock <rb...@google.com>
Gerrit-Comment-Date: Tue, 12 Jul 2022 16:38:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Roland Bock (Gerrit)

unread,
Jul 12, 2022, 3:13:14 PMJul 12
to browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, danakj, Marc Treib, Daniel Cheng, ปิยปราชญ์ สมุทรกลาง, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Enterprise Policy Reviews

Patch set 24:Commit-Queue +2

View Change

2 comments:

  • Patchset:

  • File styleguide/c++/c++.md:

    • Done

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 24
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Comment-Date: Tue, 12 Jul 2022 19:13:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes

Chromium LUCI CQ (Gerrit)

unread,
Jul 12, 2022, 4:55:50 PMJul 12
to Roland Bock, browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, danakj, Marc Treib, Daniel Cheng, ปิยปราชญ์ สมุทรกลาง, Tricium, chromium...@chromium.org, Enterprise Policy Reviews

Chromium LUCI CQ submitted this change.

View Change



23 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: styleguide/c++/c++.md
Insertions: 8, Deletions: 9.

The diff is too large to show. Please review the diff.
```

Approvals: Daniel Cheng: Looks good to me danakj: Looks good to me Marc Treib: Looks good to me Roland Bock: Commit
Add CHECK_IS_TEST

Code paths taken in tests are sometimes different from those taken in
production. This might be because the respective tests do not initialize
some objects that would be required for the "normal" code path.

Ideally, such code constructs should be avoided, so that tests really
test the production code and not something different.

However, there already are hundreds of cases, in which the cost of
avoiding special test situations was deemed too high. Cleaning up all
these cases retroactively and completely avoiding such cases in the
future seems unrealistic.

Thus, it is necessary to prevent the test code-only paths to be taken in
production scenarios.

`CHECK_IS_TEST` can be used to assert that a test-only path is actually taken
only in tests. For instance:

// Supposedly this only happens in unit tests:
if (!url_loader_factory)
{
// Assert that this code path is really only taken in test.
CHECK_IS_TEST();
return;
}

In tests, `base::test::AllowCheckIsTestToBeCalled` must be called before
`CHECK_IS_TEST`. This happens centrally in `base::TestSuite`, which
takes care of most test cases. Exceptions could be in code that gets
executed before that, e.g. in the initialization of static variables.

Both `CHECK_IS_TEST` and `base::AllowCheckIsTestToBeCalled` are thread safe.

Bug: 1326801
Test: One new unit test, plus existing tests, plus manual tests
Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3650548
Reviewed-by: danakj <dan...@chromium.org>
Commit-Queue: Roland Bock <rb...@google.com>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Marc Treib <tr...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1023402}
---
M base/BUILD.gn
A base/check_is_test.cc
A base/check_is_test.h
A base/check_is_test_unittest.cc
M base/task/sequence_manager/sequence_manager_impl.cc
M base/test/BUILD.gn
A base/test/allow_check_is_test_to_be_called.h
M base/test/launcher/unit_test_launcher.cc
M base/test/launcher/unit_test_launcher_ios.cc
M chrome/test/base/chrome_test_launcher.cc
M styleguide/c++/c++.md
11 files changed, 205 insertions(+), 0 deletions(-)


To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 25
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-MessageType: merged

Maksim Ivanov (Gerrit)

unread,
Jul 12, 2022, 5:01:33 PMJul 12
to Roland Bock, Chromium LUCI CQ, browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, danakj, Marc Treib, Daniel Cheng, ปิยปราชญ์ สมุทรกลาง, Tricium, chromium...@chromium.org, Enterprise Policy Reviews

Attention is currently required from: Roland Bock.

View Change

1 comment:

  • File base/check_is_test.h:

    • Patch Set #25, Line 37: ;

      Is this semicolon intentional? Note that it precludes using this macro in an "if"-"else" block without curly brackets. One would get something like "'else' without a previous 'if'" in a code like:

          if (...)
      CHECK_IS_TEST();
      else
      ...

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 25
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: Maksim Ivanov <em...@chromium.org>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Attention: Roland Bock <rb...@google.com>
Gerrit-Comment-Date: Tue, 12 Jul 2022 21:01:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Roland Bock (Gerrit)

unread,
Jul 14, 2022, 4:48:20 AMJul 14
to Chromium LUCI CQ, browser-comp...@chromium.org, scheduler...@chromium.org, vmpstr...@chromium.org, Maksim Ivanov, danakj, Marc Treib, Daniel Cheng, ปิยปราชญ์ สมุทรกลาง, Tricium, chromium...@chromium.org, Enterprise Policy Reviews

View Change

1 comment:

  • File base/check_is_test.h:

    • Is this semicolon intentional? Note that it precludes using this macro in an "if"-"else" block witho […]

      Awesome spot! No, not intentional. I'll create an update!

To view, visit change 3650548. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I82d70c1f72c24b3aa6c35f8a5510848989ec7df3
Gerrit-Change-Number: 3650548
Gerrit-PatchSet: 25
Gerrit-Owner: Roland Bock <rb...@google.com>
Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
Gerrit-Reviewer: Daniel Cheng <dch...@chromium.org>
Gerrit-Reviewer: Marc Treib <tr...@chromium.org>
Gerrit-Reviewer: Roland Bock <rb...@google.com>
Gerrit-Reviewer: danakj <dan...@chromium.org>
Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>
Gerrit-CC: Maksim Ivanov <em...@chromium.org>
Gerrit-CC: ปิยปราชญ์ สมุทรกลาง <phartsa...@gmail.com>
Gerrit-Comment-Date: Thu, 14 Jul 2022 08:48:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Maksim Ivanov <em...@chromium.org>
Gerrit-MessageType: comment
Reply all
Reply to author
Forward
0 new messages