Merge ~lihow731/plainbox-provider-checkbox/+git/plainbox-provider-checkbox:master into plainbox-provider-checkbox:master

[Leon Liao]

Fix the "Unable to control recording volume..." bug

[Leon Liao (lihow731)]

@Sylvain,

Could you help to view this MP?
Thank you.

[Jonathan Cave (jocave)]

I can't find any reference to this `self.default_method` attribute. Where does it come from?

review: Needs Information

[Leon Liao (lihow731)]

@Jonathan,

I correct the mistake.
Thank you.

[Jonathan Cave (jocave)]

LGTM

review: Approve

[Rex Tsai (chihchun)]

I think the fix is very wrong, it's not addressed the root cause of the issue.
Please refactor the design of _pactl_output.

review: Disapprove

[Sylvain Pineau (sylvain-pineau)]

Could you link the corresponding bug to this MR please?

review: Needs Information

[Leon Liao (lihow731)]

@Rex,

Please review the new merge proposal.

@Sylvain,
The public bug 1890586 is linked.

review: Needs Resubmitting

[Rex Tsai (chihchun)]

I would prefer to see the _pactl_output can handle the failure of pactl when it can not query or change the volume from pulseuadio.

review: Needs Fixing

[Rex Tsai (chihchun)]

Add CE QA team as reviewer, as the QA will use this test job as well.

[Leon Liao (lihow731)]

The code is modified that implemented Rex's suggestion.

review: Needs Resubmitting

[Rex Tsai (chihchun)]

review: Needs Fixing

[Leon Liao (lihow731)]

Resumbited.

review: Needs Resubmitting

[Rex Tsai (chihchun)]

Tested on focal and bionic, LGTM.

review: Approve

[Pierre Equoy (pieq)]

Thanks for fixing this!

A few comments inline (see below).

Also, regarding the commit title/message:

- Usually, the commit title includes the area that is impacted. In this case, you can prefix your commit title with `bin/audio_test.py: `
- Your commit message includes a lot of info (which is great!) but it has a few typos and I think "In get_volume() and mute(), the self.method(command) returns a zero-length string." is not correct: the `get_volume()` method does not access `self.method(command)` (I guess you meant `set_volume()`?)
- You can include `LP: #1890526` (for instance, at the very bottom of the commit message) to automatically link a merge request in Launchpad with a given Launchpad bug[1]. It's also a good thing to have in a project history.

[1] https://help.launchpad.net/Code/Git#Linking_to_bugs

review: Needs Fixing

[Leon Liao (lihow731)]

@Pierre,

I fixed and resubmit it.
Please review it.
Thanks again.

review: Needs Resubmitting

[Leon Liao (lihow731)]

About this line: self.logger.error("Unable to control recording volume. "

Original output message is:
 Unable to control recording volume.Test results may be wrong
                (There is no space ↑ )

New message:
 Unable to control recording volume. Test results may be wrong
             (There will be a space ↑ )

[Pierre Equoy (pieq)]

+1, thanks!

For the

self.logger.error("Unable to control recording volume. "
                               "Test results may be wrong")

line, I meant adding a period at the end of the sentence:

self.logger.error("Unable to control recording volume. "
                               "Test results may be wrong.")

But it's a nitpick. (and thanks for fixing the spae by the way :))

review: Approve

[StanleyHuang (stanley31)]

LGTM +1

review: Approve

[Sylvain Pineau (sylvain-pineau)]

LGTM

review: Approve

[Devices Certification Bot (ce-certification-qa)]

The merge was fine but running tests failed.

[xenial] [13:16:24] starting container
[bionic] [13:16:25] starting container
[focal] [13:16:25] starting container
Device project added to xenial-testing
Device project added to bionic-testing
Device project added to focal-testing
[xenial] [13:16:36] provisioning container
[bionic] [13:16:45] provisioning container
[focal] [13:16:45] provisioning container
[xenial] [13:16:48] Starting tests...
[xenial] Found a test script: ./requirements/container-tests-provider-checkbox
[bionic] [13:17:05] Starting tests...
[bionic] Found a test script: ./requirements/container-tests-provider-checkbox
[focal] [13:17:19] Starting tests...
[focal] Found a test script: ./requirements/container-tests-provider-checkbox
[xenial] [13:19:18] container-tests-provider-checkbox: FAIL
[xenial] output: https://paste.ubuntu.com/p/5CPFXWQnsh/
[xenial] [13:19:21] Fixing file permissions in source directory
[xenial] [13:19:22] Destroying container
[focal] [13:19:42] container-tests-provider-checkbox: FAIL
[focal] output: https://paste.ubuntu.com/p/D45PF8hkdZ/
[focal] [13:19:45] Fixing file permissions in source directory
[focal] [13:19:46] Destroying container
[bionic] [13:19:59] container-tests-provider-checkbox: FAIL
[bionic] output: https://paste.ubuntu.com/p/TxK4p4JJdx/
[bionic] [13:20:03] Fixing file permissions in source directory
[bionic] [13:20:03] Destroying container

review: Needs Fixing

[Sylvain Pineau (sylvain-pineau)]

Flake8 failed:

bin/audio_test.py:30:5: F401 'collections.Callable' imported but unused

review: Needs Fixing

[Leon Liao (lihow731)]

Updated the code to fix the Flake8 failed.

diff --git a/bin/audio_test.py b/bin/audio_test.py
index 2a56df4..4b3f89c 100755
--- a/bin/audio_test.py
+++ b/bin/audio_test.py
@@ -24,10 +24,6 @@ except ImportError:
     print((sys.version), file=sys.stderr)
     sys.exit(127)
 
-try:
-    from collections.abc import Callable
-except ImportError:
-    from collections import Callable  # backward compatible
 
 # Frequency bands for FFT
 BINS = 256
@@ -115,22 +111,16 @@ class PIDController(object):
 class PAVolumeController(object):
     pa_types = {'input': 'source', 'output': 'sink'}
 
-    def __init__(self, type, method=None, logger=None):
+    def __init__(self, type, logger=None):
         """Initializes the volume controller.
 
            Arguments:
            type: either input or output
-           method: a method that will run a command and return pulseaudio
-           information in the described format, as a single string with
-           line breaks (to be processed with str.splitlines())
 
         """
         self.type = type
         self._volume = None
         self.identifier = None
-        self.method = method
-        if not isinstance(method, Callable):
-            self.method = self._pactl_output
         self.logger = logger
 
     def set_volume(self, volume):
@@ -142,8 +132,7 @@ class PAVolumeController(object):
                    'set-%s-volume' % (self.pa_types[self.type]),
                    str(self.identifier[0]),
                    str(int(volume)) + "%"]
-        if not self.method(command):
-            return False
+        self._pactl_output(command)
         self._volume = volume
         return True
 
@@ -160,8 +149,7 @@ class PAVolumeController(object):
                    'set-%s-mute' % (self.pa_types[self.type]),
                    str(self.identifier[0]),
                    mute]
-        if not self.method(command):
-            return False
+        self._pactl_output(command)
         return True
 
     def get_identifier(self):
@@ -192,7 +180,7 @@ class PAVolumeController(object):
         # <ID>\t<NAME>\t<MODULE>\t<SAMPLE_SPEC_WITH_SPACES>\t<STATE>
         # What we need to return is the ID for the first element on this list
         # that does not contain auto_null or monitor.
-        pa_info = self.method(command)
+        pa_info = self._pactl_output(command)
         valid_elements = None
 
         if pa_info:
@@ -221,7 +209,8 @@ class PAVolumeController(object):
                                                universal_newlines=True)
             except (subprocess.CalledProcessError):
                 time.sleep(5)
-        return False
+        self.logger.error("Fail to execute: {}".format(command))
+        sys.exit(1)
 
 
 class FileDumper(object):
@@ -369,7 +358,7 @@ class GStreamerMessageHandler(object):
         # we can't control it :(
         current_volume = volume_controller.get_volume()
         if current_volume is None:
-            self.logger.error("Unable to control recording volume."
+            self.logger.error("Unable to control recording volume. "
                               "Test results may be wrong")
             return
         self.current_level = level