829561 - AudioChannelService needs some unit test

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Marco Chen [: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.

Comment 3

[Andrea Marchesini [:baku]]

> 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.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch 2

Comment 5

[Marco Chen [: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?

Comment 6

[Andrea Marchesini [:baku]]

> 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.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch c

Comment 8

[Marco Chen [: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?

Comment 9

[Andrea Marchesini [:baku]]

> 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.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch d

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 701745 [details] [diff] [review]
patch d

Ops... wrong reviewer :)

Comment 12

[Marco Chen [: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");

Comment 13

[Andrea Marchesini [:baku]]

Attachment: patch e

Yeah, all the messages were wrong.

Comment 14

[Marco Chen [:mchen]]

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

Comment 15

[Andrea Marchesini [:baku]]

Attachment: patch

Waiting for green on try + dependences landed.

Comment 16

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fca83930e09f

https://tbpl.mozilla.org/?tree=Try&rev=d5e0f1b78f54

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fca83930e09f

Comment 18

[Andrea Marchesini [:baku]]

This patch is needed for bug 830672. tef?

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Not blocking tef on test only patch, this can be landed to branches when ready since it's nptob.

Comment 20

[Andrea Marchesini [:baku]]

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+.

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 702183 [details] [diff] [review]
patch

Requesting approval since it blocks approved bug 830672 from being uplifted.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5e5b35cd7f5d

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 25

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0a2501533cfd
The 830648 was the reason why this patch failed.