AudioChannelService needs some unit test

RESOLVED FIXED in Firefox 21

Status

()

defect
RESOLVED FIXED
6 years ago
a month ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla21
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:-, firefox18+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #701027 - Flags: review?(mchen)
Comment on attachment 701027 [details] [diff] [review]
patch

Review of attachment 701027 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your help on unit test. I learn from you.

::: dom/audiochannel/tests/TestAudioChannelService.cpp
@@ +9,5 @@
> +
> +#define TEST_ENSURE_BASE(_test, _msg)       \
> +  PR_BEGIN_MACRO                            \
> +    if (_test) {                            \
> +      fail(_msg);                           \

Refe to line 56, the correct value of muted should be true.
Then the true in this macro will be triggerred as fail case.
Is it should be "if(!_test)"?

@@ +128,5 @@
> +  TEST_ENSURE_BASE(!muted, "Test3: A content channel unvisible agent1 should not be muted");
> +
> +  rv = agent2.mAgent->StartPlaying(&muted);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  TEST_ENSURE_BASE(!muted, "Test3: A content channel unvisible agent2 should not be muted");

As I remember that, the current design on audiochannelservice will mute content channels if there are more then one in the backround.

@@ +149,5 @@
> +
> +  rv = agent2.mAgent->StartPlaying(&muted);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  TEST_ENSURE_BASE(!muted, "Test3: A content channel visible agent2 should not be muted");
> +

Do we need add a case for one in the foreground (not muted) and one in the backround (muted).

@@ +197,5 @@
> +
> +  rv = alarmAgent.mAgent->StartPlaying(&muted);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  TEST_ENSURE_BASE(muted, "Test3: A alarm channel unvisible agent should be muted when telephony is playing");
> +

I also suggest to test the relationship between one content and one publicnotification channel. For case of camera shutter sound, we need publicnotification to mute content channel in the background.
(Assignee)

Comment 3

6 years ago
> Refe to line 56, the correct value of muted should be true.
> Then the true in this macro will be triggerred as fail case.
> Is it should be "if(!_test)"?

Right. I just realized that StartPlaying returns !muted :)
I'm going to renamed muted to playing.


> As I remember that, the current design on audiochannelservice will mute
> content channels if there are more then one in the backround.

Right. But in this case they are coming from the same process, so they are allowed.

> Do we need add a case for one in the foreground (not muted) and one in the
> backround (muted).

ok

> I also suggest to test the relationship between one content and one
> publicnotification channel. For case of camera shutter sound, we need
> publicnotification to mute content channel in the background.

ok!
New patch will be posted soon.
(Assignee)

Comment 4

6 years ago
Posted patch patch 2 (obsolete) — Splinter Review
Attachment #701027 - Attachment is obsolete: true
Attachment #701027 - Flags: review?(mchen)
Attachment #701131 - Flags: review?(mchen)
Comment on attachment 701131 [details] [diff] [review]
patch 2

Review of attachment 701131 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/audiochannel/tests/TestAudioChannelService.cpp
@@ +9,5 @@
> +
> +#define TEST_ENSURE_BASE(_test, _msg)       \
> +  PR_BEGIN_MACRO                            \
> +    if (_test) {                            \
> +      fail(_msg);                           \

May I make sure that this test is failed when code is reached here?
If the answer is yes, then I will expect all of _test passed into here are "false".
Is it right?

@@ +52,5 @@
> +
> +  bool playing;
> +  rv = agent.mAgent->StartPlaying(&playing);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  TEST_ENSURE_BASE(playing, "Test1: A normal channel unvisible agent must be playing");

I want to clarify the meaning of argument in StartPlaying(bool canPlay) first.
If true, it means the media associated to this agent is allowed to play. (equal to getMuted() returned false)
If false, it is not allowed to play. (equal to getMuted() returned true)

So variable - playing here will be false because normal channel in background is not allowed to play.
Since msg here is used when test case is failed, should it be
  "Test1: A normal channel unvisible agent must |not| be playing".
Or "Test1: An agent associated with normal channel in background must not be allowed to play"

@@ +202,5 @@
> +
> +  rv = normalAgent.mAgent->StartPlaying(&playing);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  TEST_ENSURE_BASE(!playing, "Test3: A normal channel visible agent should not be playing");
> +

Does here assume that the patch of bug 828200 is already landed?
(Assignee)

Comment 6

6 years ago
> May I make sure that this test is failed when code is reached here?
> If the answer is yes, then I will expect all of _test passed into here are
> "false".
> Is it right?

Yes.

> Does here assume that the patch of bug 828200 is already landed?

Yep.
I'm uploading a new patch with a better handle for !playing + messages.
(Assignee)

Comment 7

6 years ago
Posted patch patch c (obsolete) — Splinter Review
Attachment #701131 - Attachment is obsolete: true
Attachment #701131 - Flags: review?(mchen)
Attachment #701735 - Flags: review?(mchen)
Comment on attachment 701735 [details] [diff] [review]
patch c

Review of attachment 701735 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for you to take my consideration.

::: dom/audiochannel/tests/TestAudioChannelService.cpp
@@ +70,5 @@
> +  return rv;
> +}
> +
> +nsresult
> +TestTwoNormalChannels()

Is any special case we need to test two agents with normal channel?
I think it is no special then only one normal channel in foreground or background.

@@ +150,5 @@
> +  TEST_ENSURE_BASE(playing, "Test3: A content channel visible agent1 should be playing");
> +
> +  rv = agent2.mAgent->StartPlaying(&playing);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  TEST_ENSURE_BASE(!playing, "Test3: A content channel visible agent2 should not be playing");

Test3: A content channel |unvisible| agent2 should not be playing

@@ +177,5 @@
> +}
> +
> +nsresult
> +TestPriorities()
> +{

Since here is used to test priorities, do we need to test from content to publicnotification channels?
Ex: 1. playing content in background
    2. playing notification then check content
    3. playing alarm then check notification and content
    4. so on ...

    n. playing publicnotification then check ring, telephony, alarm, notification and content

@@ +252,5 @@
> +
> +  rv = normalAgent.mAgent->StartPlaying(&playing);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  TEST_ENSURE_BASE(playing, "Test3: A normal channel visible agent should be playing");
> +

The test until here is exactly the same with TestPriorities().
Or we just add a new agent for publicnotification into TestPriorities() for testing?
(Assignee)

Comment 9

6 years ago
> Is any special case we need to test two agents with normal channel?
> I think it is no special then only one normal channel in foreground or
> background.

Right. But it's just 1 test more :)

> @@ +150,5 @@
> > +  TEST_ENSURE_BASE(playing, "Test3: A content channel visible agent1 should be playing");
> > +
> > +  rv = agent2.mAgent->StartPlaying(&playing);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  TEST_ENSURE_BASE(!playing, "Test3: A content channel visible agent2 should not be playing");
> 
> Test3: A content channel |unvisible| agent2 should not be playing

Actually, this is right. 2 content channels can play if it's the same child ID (~== appId).
Think about an app that uses 2 content channels, they should play together also if in background.

> Since here is used to test priorities, do we need to test from content to
> publicnotification channels?
> Ex: 1. playing content in background
>     2. playing notification then check content
>     3. playing alarm then check notification and content
>     4. so on ...

Right.


> The test until here is exactly the same with TestPriorities().
> Or we just add a new agent for publicnotification into TestPriorities() for
> testing?

Yep.
(Assignee)

Comment 10

6 years ago
Posted patch patch d (obsolete) — Splinter Review
Attachment #701735 - Attachment is obsolete: true
Attachment #701735 - Flags: review?(mchen)
Attachment #701745 - Flags: review?(jonas)
(Assignee)

Comment 11

6 years ago
Comment on attachment 701745 [details] [diff] [review]
patch d

Ops... wrong reviewer :)
Attachment #701745 - Flags: review?(jonas) → review?(mchen)
Comment on attachment 701745 [details] [diff] [review]
patch d

Review of attachment 701745 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the modification.

::: dom/audiochannel/tests/TestAudioChannelService.cpp
@@ +150,5 @@
> +  TEST_ENSURE_BASE(playing, "Test3: A content channel visible agent1 should be playing");
> +
> +  rv = agent2.mAgent->StartPlaying(&playing);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  TEST_ENSURE_BASE(!playing, "Test3: A content channel visible agent2 should not be playing");

Since line 145 already set visibility of agent2 to false then the message here should be
  unvisible agent 2 not visible agent 2

@@ +224,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  TEST_ENSURE_BASE(playing, "Test4: An notification channel unvisible agent should be playing");
> +
> +  // Now content should be not playing because of the notification playing.
> +  rv = contentAgent.mAgent->StartPlaying(&playing);

We need to stopPlaying first then calling StartPlaying again.
The same on line 238, 248, 258, 268.

Note: I should fire a bug for preventing one agent from double registering into service.

@@ +233,5 @@
> +  rv = alarmAgent.mAgent->StartPlaying(&playing);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  TEST_ENSURE_BASE(playing, "Test4: An alarm channel unvisible agent should be playing");
> +
> +  // Now notification should be not playing because of the notification playing.

// Now notification should be not playing because of the |alarm| playing.

@@ +243,5 @@
> +  rv = telephonyAgent.mAgent->StartPlaying(&playing);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  TEST_ENSURE_BASE(playing, "Test4: An telephony channel unvisible agent should be playing");
> +
> +  // Now alarm should be not playing because of the alarm playing.

// Now alarm should be not playing because of the |telephony| playing.

@@ +253,5 @@
> +  rv = ringerAgent.mAgent->StartPlaying(&playing);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  TEST_ENSURE_BASE(playing, "Test4: An ringer channel unvisible agent should be playing");
> +
> +  // Now telephony should be not playing because of the telephony playing.

// Now telephony should be not playing because of the |ringer| playing.

@@ +256,5 @@
> +
> +  // Now telephony should be not playing because of the telephony playing.
> +  rv = telephonyAgent.mAgent->StartPlaying(&playing);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  TEST_ENSURE_BASE(!playing, "Test4: A telephony channel unvisible agent should not be playing when a telephony is playing");

"Test4: A telephony channel unvisible agent should not be playing when a |ringer| is playing"

@@ +263,5 @@
> +  rv = pNotificationAgent.mAgent->StartPlaying(&playing);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  TEST_ENSURE_BASE(playing, "Test4: An pNotification channel unvisible agent should be playing");
> +
> +  // Now ringer should be not playing because of the ringer playing.

// Now ringer should be not playing because of the |pNotification| playing.

@@ +266,5 @@
> +
> +  // Now ringer should be not playing because of the ringer playing.
> +  rv = ringerAgent.mAgent->StartPlaying(&playing);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  TEST_ENSURE_BASE(!playing, "Test4: A ringer channel unvisible agent should not be playing when a ringer is playing");

TEST_ENSURE_BASE(!playing, "Test4: A ringer channel unvisible agent should not be playing when a |publicNotification| is playing");
(Assignee)

Updated

6 years ago
Depends on: 830648
(Assignee)

Comment 13

6 years ago
Posted patch patch e (obsolete) — Splinter Review
Yeah, all the messages were wrong.
Attachment #701745 - Attachment is obsolete: true
Attachment #701745 - Flags: review?(mchen)
Attachment #702170 - Flags: review?(mchen)
(Assignee)

Updated

6 years ago
Depends on: 828200
Comment on attachment 702170 [details] [diff] [review]
patch e

Review of attachment 702170 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great.
It is review+ after fixing the wrong wording.

::: dom/audiochannel/tests/TestAudioChannelService.cpp
@@ +58,5 @@
> +  bool mRegistered;
> +};
> +
> +nsresult
> +TestDoupleStartPlaying()

TestDoubleStartPlaying
Attachment #702170 - Flags: review?(mchen) → review+
(Assignee)

Comment 15

6 years ago
Posted patch patchSplinter Review
Waiting for green on try + dependences landed.
Attachment #702170 - Attachment is obsolete: true
Attachment #702183 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/fca83930e09f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Assignee)

Comment 18

6 years ago
This patch is needed for bug 830672. tef?
blocking-b2g: --- → tef?
Not blocking tef on test only patch, this can be landed to branches when ready since it's nptob.
blocking-b2g: tef? → -
(Assignee)

Comment 20

6 years ago
This patch is not just about testing. It defines CONTENT_PARENT_NO_CHILD_ID in ContentParent.h and uses this definition in AudioChannelService.cpp. This fixes a small bug that has been shown by the test implemented by this patch.

Please, reconsider the tef+.
blocking-b2g: - → tef?
If, as you say, this fixes a small bug then again it's still not a blocker but it's already tracked for b2g18 and if the patch is nominated for approval and is low risk we can still take it for uplift (similar to bug 830672 which is also waiting for justification of last-minute landing). Alternately it can go into a v1-train.
blocking-b2g: tef? → -
Comment on attachment 702183 [details] [diff] [review]
patch

Requesting approval since it blocks approved bug 830672 from being uplifted.
Attachment #702183 - Flags: approval-mozilla-b2g18?
Attachment #702183 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Backed out for test failures:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d139dca8c3b4

https://tbpl.mozilla.org/php/getParsedLog.php?id=19098781&tree=Mozilla-B2g18

Running AudioChannelService tests...
TEST-UNEXPECTED-FAIL | Test0: StartPlaying calling twice should return error
Finished running AudioChannelService tests.
(Assignee)

Comment 25

6 years ago
https://hg.mozilla.org/releases/mozilla-b2g18/rev/0a2501533cfd
The 830648 was the reason why this patch failed.
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.