830408 - Tests for data reporting notification bar

Status: RESOLVED FIXED

(Firefox :: General, defect, P4)

Description

[Gregory Szorc [:gps]]

The data reporting notification bar added in bug 804745 does not have any automated test coverage. We should definitely add tests for it!

Comment 1

[Gregory Szorc [:gps]]

Attachment: Tests, v1

Not yet finished so not asking for review.

I rarely write mochitests and my DOM skills are lacking. Just looking for feedback to tell me if I'm on the right track and if there is a better way I could do some of this.

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 710893 [details] [diff] [review]
Tests, v1

>+  let notification = document.getElementById("global-notificationbox");
>+  ok(!notification.notificationsHidden, "Notification box is hidden by default.");

ok(!something.hidden, "something is hidden") looks backwards. I also think you're misunderstanding the notificationsHidden property. It's not supposed to tell you that there are no notifications, but that they're temporarily hidden (e.g. in print preview mode).

>+  notification.addEventListener("AlertActive", function active() {
>+    notification.removeEventListener("AlertActive", active, true);

This appears to be obsolete / dead code.

Comment 3

[Gregory Szorc [:gps]]

(In reply to Dão Gottwald [:dao] from comment #2)
> Comment on attachment 710893 [details] [diff] [review]
> Tests, v1
> 
> >+  let notification = document.getElementById("global-notificationbox");
> >+  ok(!notification.notificationsHidden, "Notification box is hidden by default.");
> 
> ok(!something.hidden, "something is hidden") looks backwards. I also think
> you're misunderstanding the notificationsHidden property. It's not supposed
> to tell you that there are no notifications, but that they're temporarily
> hidden (e.g. in print preview mode).

It appears so. I'll remove the test since it seems worthless.

> >+  notification.addEventListener("AlertActive", function active() {
> >+    notification.removeEventListener("AlertActive", active, true);
> 
> This appears to be obsolete / dead code.

This is most unfortunate. Testing this code would be much easier if AlertActive and AlertClose were available. IMO we should add and support these.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Tests for data notification bar, v2

While I was here, I also fixed a bug where notification bars in multiple windows would send a policy acceptance update on receipt of a close request. Now, the notification only updates policy status if it itself was acted upon.

Comment 5

[Gregory Szorc [:gps]]

Gavin: review ping. Feel free to 301.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 717199 [details] [diff] [review]
Tests for data notification bar, v2

Can you add a comment in the datareporting:notify-data-policy:close observer that you're setting actionTaken not because an action was actually taken on this notification bar, but because the observer firing means that an action was taken on another, and it would have taken care of calling request.onUserAccept so that you shouldn't? I tried to think of a better name for "actionTaken" that would make that clear, but I don't think there is one; this code is a bit confusing.

Comment 7

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/9aba82c4908b

Comment 8

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/9aba82c4908b