New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 733948 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Feature

Blocking:
issue 723144



Sign in to add a comment

Add unit tests / browser tests for new-style notifications.

Project Member Reported by fukino@chromium.org, Jun 16 2017

Issue description

We should be able to reuse the tests for previous notifications to some extent.
 
Labels: -Type-Bug Type-Feature
Owner: tetsui@chromium.org
Status: Assigned (was: Available)
Is anyone working on this? I'm thinking of starting it by porting some of the unit test cases from

https://cs.chromium.org/chromium/src/ui/message_center/views/notification_view_unittest.cc

Feel free to take this item back. Thanks!
I think no one working on this. Thank you for taking this!
Status: Started (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9a4a1121fae26a1727728888a41975bc15e3e62f

commit 9a4a1121fae26a1727728888a41975bc15e3e62f
Author: tetsui <tetsui@chromium.org>
Date: Fri Jul 07 07:27:10 2017

Port NotificationViewMD unit tests from NotificationViewTest.

Ported unit tests from NotificationViewTest and created
NotificationViewMDTest. Also fixed bugs and check failures found during
porting the tests.
* Fix RemoveChildView null checking. (CHECK fails on debug builds.)
* Fix action button hover state inheritance.

Following tests are not ported from NotificationViewTest, mostly
because the feature is unimplemented or the test is not applicable.
* CreateOrUpdateTestSettingsButton
* TestLineLimits
* TestImageSizing
* SettingsButtonTest
* ViewOrderingTest
* FormatContextMessageTest

BUG= 733948 
TEST=out/Debug/message_center_unittests

Review-Url: https://codereview.chromium.org/2966343002
Cr-Commit-Position: refs/heads/master@{#484851}

[modify] https://crrev.com/9a4a1121fae26a1727728888a41975bc15e3e62f/ui/message_center/BUILD.gn
[modify] https://crrev.com/9a4a1121fae26a1727728888a41975bc15e3e62f/ui/message_center/views/notification_view_md.cc
[modify] https://crrev.com/9a4a1121fae26a1727728888a41975bc15e3e62f/ui/message_center/views/notification_view_md.h
[add] https://crrev.com/9a4a1121fae26a1727728888a41975bc15e3e62f/ui/message_center/views/notification_view_md_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a0e04c835cae01af85b8532022cec6ee5346f69f

commit a0e04c835cae01af85b8532022cec6ee5346f69f
Author: mastiz <mastiz@chromium.org>
Date: Fri Jul 07 12:45:44 2017

Revert of Port NotificationViewMD unit tests from NotificationViewTest. (patchset #5 id:80001 of https://codereview.chromium.org/2966343002/ )

Reason for revert:
message_center_unittests failing on chromium.memory/Linux Chromium OS ASan LSan Tests (1)

BUG= 739356 

Original issue's description:
> Port NotificationViewMD unit tests from NotificationViewTest.
>
> Ported unit tests from NotificationViewTest and created
> NotificationViewMDTest. Also fixed bugs and check failures found during
> porting the tests.
> * Fix RemoveChildView null checking. (CHECK fails on debug builds.)
> * Fix action button hover state inheritance.
>
> Following tests are not ported from NotificationViewTest, mostly
> because the feature is unimplemented or the test is not applicable.
> * CreateOrUpdateTestSettingsButton
> * TestLineLimits
> * TestImageSizing
> * SettingsButtonTest
> * ViewOrderingTest
> * FormatContextMessageTest
>
> BUG= 733948 
> TEST=out/Debug/message_center_unittests
>
> Review-Url: https://codereview.chromium.org/2966343002
> Cr-Commit-Position: refs/heads/master@{#484851}
> Committed: https://chromium.googlesource.com/chromium/src/+/9a4a1121fae26a1727728888a41975bc15e3e62f

TBR=fukino@chromium.org,yoshiki@chromium.org,tetsui@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 733948 

Review-Url: https://codereview.chromium.org/2965353002
Cr-Commit-Position: refs/heads/master@{#484899}

[modify] https://crrev.com/a0e04c835cae01af85b8532022cec6ee5346f69f/ui/message_center/BUILD.gn
[modify] https://crrev.com/a0e04c835cae01af85b8532022cec6ee5346f69f/ui/message_center/views/notification_view_md.cc
[modify] https://crrev.com/a0e04c835cae01af85b8532022cec6ee5346f69f/ui/message_center/views/notification_view_md.h
[delete] https://crrev.com/dbb008f0e650d625766f8d29776a52aadbad9ce1/ui/message_center/views/notification_view_md_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/67cb979431064350778e059b4204e7241cad00b7

commit 67cb979431064350778e059b4204e7241cad00b7
Author: Tetsui Ohkubo <tetsui@chromium.org>
Date: Tue Jul 11 11:46:47 2017

Fix NotificationViewMD leak and restore unit tests.

NotificationViewMDTest were reverted due to LSan failure in
NotificationViewMD.
This CL fixes the leak and restores NotificationViewMDTest.
The leak was caused because we assumed a->RemoveChildView(b)
would delete the b object, but it was not true.
We should use DCHECK(a->Contains(b)); delete b; instead.

Original CL: https://codereview.chromium.org/2966343002/

BUG= 733948 , 739356 
TEST=
gn gen out/asan --args='is_asan=true is_lsan=true enable_nacl=false
is_debug=false' &&
ASAN_OPTIONS="detect_leaks=1 symbolize=1
external_symbolizer_path=`pwd`/third_party/llvm-build/
Release+Asserts/bin/llvm-symbolizer" out/asan/message_center_unittests

Change-Id: I81f1aefdab6fafa35fd00e69875b1aa426822c9e
Reviewed-on: https://chromium-review.googlesource.com/564859
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485591}
[modify] https://crrev.com/67cb979431064350778e059b4204e7241cad00b7/ui/message_center/BUILD.gn
[modify] https://crrev.com/67cb979431064350778e059b4204e7241cad00b7/ui/message_center/views/notification_view_md.cc
[modify] https://crrev.com/67cb979431064350778e059b4204e7241cad00b7/ui/message_center/views/notification_view_md.h
[add] https://crrev.com/67cb979431064350778e059b4204e7241cad00b7/ui/message_center/views/notification_view_md_unittest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/282086c89278d5ba5701e1089889e2c919c761c6

commit 282086c89278d5ba5701e1089889e2c919c761c6
Author: Tetsui Ohkubo <tetsui@chromium.org>
Date: Wed Aug 16 09:17:45 2017

Add unit test of new style notification accent color.

This CL adds unit test for NotificationViewMD's representation of
|accent_color|.
In order to expose variables for testing, NotificationButtonMD is moved.

TEST=out/Debug/message_center_unittests
BUG= 733948 

Change-Id: Ib273e0f23ee5becf38b4788ad2288546c65d689c
Reviewed-on: https://chromium-review.googlesource.com/615206
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494732}
[modify] https://crrev.com/282086c89278d5ba5701e1089889e2c919c761c6/ui/message_center/views/notification_header_view.h
[modify] https://crrev.com/282086c89278d5ba5701e1089889e2c919c761c6/ui/message_center/views/notification_view_md.cc
[modify] https://crrev.com/282086c89278d5ba5701e1089889e2c919c761c6/ui/message_center/views/notification_view_md.h
[modify] https://crrev.com/282086c89278d5ba5701e1089889e2c919c761c6/ui/message_center/views/notification_view_md_unittest.cc

Comment 8 by tetsui@chromium.org, Jun 20 2018

Status: Archived (was: Started)

Sign in to add a comment