Modifying Makefile.am to facilitate test-adv-monitor and future unit tests.

Modifying Makefile.am to facilitate test-adv-monitor and future unit tests.

[Miao-chen Chou]

Hi Luiz and Marcel,

Unlike the rest of the existing unit tests in BlueZ, the logic blocks
tested in test-adv-monitor require dependencies of not only
src/adv_monitor.c but also all the dependency tree of
src/adv_monitor.c. The current convention in Makefile.am is to add all
the extra dependencies one by one. However, the maintenance cost is
high and not suitable in the case of test-adv-monitor. Therefore, we'd
like to propose changes in Makefile.am to make the source of
bluetoothd as a static library and link it for bluetoothd target and
the unit test target. It would be great if you can provide feedback on
this idea before the implementation. Thanks in advance!

Regards,
Miao

Re: Modifying Makefile.am to facilitate test-adv-monitor and future unit tests.

[Luiz Augusto von Dentz]

Hi Miao,

On Fri, Sep 18, 2020 at 12:44 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> Hi Luiz and Marcel,
>
> Unlike the rest of the existing unit tests in BlueZ, the logic blocks
> tested in test-adv-monitor require dependencies of not only
> src/adv_monitor.c but also all the dependency tree of
> src/adv_monitor.c. The current convention in Makefile.am is to add all
> the extra dependencies one by one. However, the maintenance cost is
> high and not suitable in the case of test-adv-monitor. Therefore, we'd
> like to propose changes in Makefile.am to make the source of
> bluetoothd as a static library and link it for bluetoothd target and
> the unit test target. It would be great if you can provide feedback on
> this idea before the implementation. Thanks in advance!

Then we should have had the code move to src/shared for unit testing,
but how exactly are you planning to do that? For testing the kernel
interface it normally done via a dedicated tester, but that again can
be done with shared library.

> Regards,
> Miao



-- 
Luiz Augusto von Dentz

Re: Modifying Makefile.am to facilitate test-adv-monitor and future unit tests.

[Miao-chen Chou]

Hi Luiz,

On Fri, Sep 18, 2020 at 3:12 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Miao,
>
> On Fri, Sep 18, 2020 at 12:44 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> >
> > Hi Luiz and Marcel,
> >
> > Unlike the rest of the existing unit tests in BlueZ, the logic blocks
> > tested in test-adv-monitor require dependencies of not only
> > src/adv_monitor.c but also all the dependency tree of
> > src/adv_monitor.c. The current convention in Makefile.am is to add all
> > the extra dependencies one by one. However, the maintenance cost is
> > high and not suitable in the case of test-adv-monitor. Therefore, we'd
> > like to propose changes in Makefile.am to make the source of
> > bluetoothd as a static library and link it for bluetoothd target and
> > the unit test target. It would be great if you can provide feedback on
> > this idea before the implementation. Thanks in advance!
>
> Then we should have had the code move to src/shared for unit testing,
> but how exactly are you planning to do that? For testing the kernel
> interface it normally done via a dedicated tester, but that again can
> be done with shared library.
>
In series https://patchwork.kernel.org/project/bluetooth/list/?series=351021,
we introduced some helper functions in adv_monitor.h to perform unit
testing and test-adv-monitor to facilitate the unit tests of
adv_monitor. We are encountering an expected build check failure on
this series.

There are two categories in test-adv-monitor, content filtering and
RSSI tracking, and content filter is easy to be moved to a standalone
shared component while RSSI tracking involves the use of timer, D-Bus
method calls and adv_monitor's internal structures, and that makes it
strongly coupled with the adv_monitor implementation which require a
tree of dependencies apart from adv_monitor.

There are two options to resolve the build failures in our case.
(1) Reorganize Makefile.am
This option is to make the sources (except src/main.c) into a static
lib and link this lib in bluetoothd executable target and whichever
unit test the sources are required.
(2) Create src/shared/am to facilitate helpers and core logic
This option is to create a new source under src/shared/ to facilitate
helper functions and core logic for src/adv_monitor. The interface of
src/shared/am may have the following functions.
- Create/destroy functions of struct adv_monitor
- Create/destroy functions of struct adv_monitor_device
- Helper function for monitor content matching.
- Helper function for RSSI tracking
However, the logic of RSSI tracking is hard to be ripped off and moved
to src/shared/am. One example would be the use of timer in RSSI
tracking, and there is currently no previous example of the timer use
in the shared library.

Series https://patchwork.kernel.org/project/bluetooth/list/?series=351021
is up for review. Our next step here would be ripped off the unit test
for now and submit v5 of the series. Once we reach an conclusion on
advmon unit test, we can submit a separate series including the
refactoring and unit tests. Looking forward to any feedbacks. Thanks!

Regards,
Miao

Re: Modifying Makefile.am to facilitate test-adv-monitor and future unit tests.

[Luiz Augusto von Dentz]

Hi Miao,

On Mon, Sep 21, 2020 at 12:21 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> Hi Luiz,
>
> On Fri, Sep 18, 2020 at 3:12 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Miao,
> >
> > On Fri, Sep 18, 2020 at 12:44 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> > >
> > > Hi Luiz and Marcel,
> > >
> > > Unlike the rest of the existing unit tests in BlueZ, the logic blocks
> > > tested in test-adv-monitor require dependencies of not only
> > > src/adv_monitor.c but also all the dependency tree of
> > > src/adv_monitor.c. The current convention in Makefile.am is to add all
> > > the extra dependencies one by one. However, the maintenance cost is
> > > high and not suitable in the case of test-adv-monitor. Therefore, we'd
> > > like to propose changes in Makefile.am to make the source of
> > > bluetoothd as a static library and link it for bluetoothd target and
> > > the unit test target. It would be great if you can provide feedback on
> > > this idea before the implementation. Thanks in advance!
> >
> > Then we should have had the code move to src/shared for unit testing,
> > but how exactly are you planning to do that? For testing the kernel
> > interface it normally done via a dedicated tester, but that again can
> > be done with shared library.
> >
> In series https://patchwork.kernel.org/project/bluetooth/list/?series=351021,
> we introduced some helper functions in adv_monitor.h to perform unit
> testing and test-adv-monitor to facilitate the unit tests of
> adv_monitor. We are encountering an expected build check failure on
> this series.
>
> There are two categories in test-adv-monitor, content filtering and
> RSSI tracking, and content filter is easy to be moved to a standalone
> shared component while RSSI tracking involves the use of timer, D-Bus
> method calls and adv_monitor's internal structures, and that makes it
> strongly coupled with the adv_monitor implementation which require a
> tree of dependencies apart from adv_monitor.
>
> There are two options to resolve the build failures in our case.
> (1) Reorganize Makefile.am
> This option is to make the sources (except src/main.c) into a static
> lib and link this lib in bluetoothd executable target and whichever
> unit test the sources are required.
> (2) Create src/shared/am to facilitate helpers and core logic
> This option is to create a new source under src/shared/ to facilitate
> helper functions and core logic for src/adv_monitor. The interface of
> src/shared/am may have the following functions.
> - Create/destroy functions of struct adv_monitor
> - Create/destroy functions of struct adv_monitor_device
> - Helper function for monitor content matching.
> - Helper function for RSSI tracking
> However, the logic of RSSI tracking is hard to be ripped off and moved
> to src/shared/am. One example would be the use of timer in RSSI
> tracking, and there is currently no previous example of the timer use
> in the shared library.

Not really following regarding the dependency on D-Bus, usually shared
don't have dependency on that because the code would be part of the
deamon and in that case you would be better of  testing that direct
via a tester that does exercise its D-Bus API. Lets be clear here,
except for gbus itself all our tests under unit directory are for C
function testing, if you want to test a D-Bus interface then we need a
tester that would do that over D-Bus.

> Series https://patchwork.kernel.org/project/bluetooth/list/?series=351021
> is up for review. Our next step here would be ripped off the unit test
> for now and submit v5 of the series. Once we reach an conclusion on
> advmon unit test, we can submit a separate series including the
> refactoring and unit tests. Looking forward to any feedbacks. Thanks!

I'd strip the testing for now since it doesn't seem we are on sync to
how we test things apparently.

> Regards,
> Miao



-- 
Luiz Augusto von Dentz

Re: Modifying Makefile.am to facilitate test-adv-monitor and future unit tests.

[Miao-chen Chou]

Hi Luiz,


On Mon, Sep 21, 2020 at 6:44 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Miao,
>
> On Mon, Sep 21, 2020 at 12:21 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> >
> > Hi Luiz,
> >
> > On Fri, Sep 18, 2020 at 3:12 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Miao,
> > >
> > > On Fri, Sep 18, 2020 at 12:44 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> > > >
> > > > Hi Luiz and Marcel,
> > > >
> > > > Unlike the rest of the existing unit tests in BlueZ, the logic blocks
> > > > tested in test-adv-monitor require dependencies of not only
> > > > src/adv_monitor.c but also all the dependency tree of
> > > > src/adv_monitor.c. The current convention in Makefile.am is to add all
> > > > the extra dependencies one by one. However, the maintenance cost is
> > > > high and not suitable in the case of test-adv-monitor. Therefore, we'd
> > > > like to propose changes in Makefile.am to make the source of
> > > > bluetoothd as a static library and link it for bluetoothd target and
> > > > the unit test target. It would be great if you can provide feedback on
> > > > this idea before the implementation. Thanks in advance!
> > >
> > > Then we should have had the code move to src/shared for unit testing,
> > > but how exactly are you planning to do that? For testing the kernel
> > > interface it normally done via a dedicated tester, but that again can
> > > be done with shared library.
> > >
> > In series https://patchwork.kernel.org/project/bluetooth/list/?series=351021,
> > we introduced some helper functions in adv_monitor.h to perform unit
> > testing and test-adv-monitor to facilitate the unit tests of
> > adv_monitor. We are encountering an expected build check failure on
> > this series.
> >
> > There are two categories in test-adv-monitor, content filtering and
> > RSSI tracking, and content filter is easy to be moved to a standalone
> > shared component while RSSI tracking involves the use of timer, D-Bus
> > method calls and adv_monitor's internal structures, and that makes it
> > strongly coupled with the adv_monitor implementation which require a
> > tree of dependencies apart from adv_monitor.
> >
> > There are two options to resolve the build failures in our case.
> > (1) Reorganize Makefile.am
> > This option is to make the sources (except src/main.c) into a static
> > lib and link this lib in bluetoothd executable target and whichever
> > unit test the sources are required.
> > (2) Create src/shared/am to facilitate helpers and core logic
> > This option is to create a new source under src/shared/ to facilitate
> > helper functions and core logic for src/adv_monitor. The interface of
> > src/shared/am may have the following functions.
> > - Create/destroy functions of struct adv_monitor
> > - Create/destroy functions of struct adv_monitor_device
> > - Helper function for monitor content matching.
> > - Helper function for RSSI tracking
> > However, the logic of RSSI tracking is hard to be ripped off and moved
> > to src/shared/am. One example would be the use of timer in RSSI
> > tracking, and there is currently no previous example of the timer use
> > in the shared library.
>
> Not really following regarding the dependency on D-Bus, usually shared
> don't have dependency on that because the code would be part of the
> deamon and in that case you would be better of  testing that direct
> via a tester that does exercise its D-Bus API. Lets be clear here,
> except for gbus itself all our tests under unit directory are for C
> function testing, if you want to test a D-Bus interface then we need a
> tester that would do that over D-Bus.
>
> > Series https://patchwork.kernel.org/project/bluetooth/list/?series=351021
> > is up for review. Our next step here would be ripped off the unit test
> > for now and submit v5 of the series. Once we reach an conclusion on
> > advmon unit test, we can submit a separate series including the
> > refactoring and unit tests. Looking forward to any feedbacks. Thanks!
>
> I'd strip the testing for now since it doesn't seem we are on sync to
> how we test things apparently.
>
Thanks for your reply. We need to rethink how to perform the testing.
And agree, we do plan to strip the testing for now from our series.
> > Regards,
> > Miao
>
>
>
> --
> Luiz Augusto von Dentz

Re: Modifying Makefile.am to facilitate test-adv-monitor and future unit tests.

[Luiz Augusto von Dentz]

Hi Miao,

On Thu, Sep 24, 2020 at 1:44 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> Hi Luiz,
>
>
> On Mon, Sep 21, 2020 at 6:44 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Miao,
> >
> > On Mon, Sep 21, 2020 at 12:21 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> > >
> > > Hi Luiz,
> > >
> > > On Fri, Sep 18, 2020 at 3:12 PM Luiz Augusto von Dentz
> > > <luiz.dentz@gmail.com> wrote:
> > > >
> > > > Hi Miao,
> > > >
> > > > On Fri, Sep 18, 2020 at 12:44 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> > > > >
> > > > > Hi Luiz and Marcel,
> > > > >
> > > > > Unlike the rest of the existing unit tests in BlueZ, the logic blocks
> > > > > tested in test-adv-monitor require dependencies of not only
> > > > > src/adv_monitor.c but also all the dependency tree of
> > > > > src/adv_monitor.c. The current convention in Makefile.am is to add all
> > > > > the extra dependencies one by one. However, the maintenance cost is
> > > > > high and not suitable in the case of test-adv-monitor. Therefore, we'd
> > > > > like to propose changes in Makefile.am to make the source of
> > > > > bluetoothd as a static library and link it for bluetoothd target and
> > > > > the unit test target. It would be great if you can provide feedback on
> > > > > this idea before the implementation. Thanks in advance!
> > > >
> > > > Then we should have had the code move to src/shared for unit testing,
> > > > but how exactly are you planning to do that? For testing the kernel
> > > > interface it normally done via a dedicated tester, but that again can
> > > > be done with shared library.
> > > >
> > > In series https://patchwork.kernel.org/project/bluetooth/list/?series=351021,
> > > we introduced some helper functions in adv_monitor.h to perform unit
> > > testing and test-adv-monitor to facilitate the unit tests of
> > > adv_monitor. We are encountering an expected build check failure on
> > > this series.
> > >
> > > There are two categories in test-adv-monitor, content filtering and
> > > RSSI tracking, and content filter is easy to be moved to a standalone
> > > shared component while RSSI tracking involves the use of timer, D-Bus
> > > method calls and adv_monitor's internal structures, and that makes it
> > > strongly coupled with the adv_monitor implementation which require a
> > > tree of dependencies apart from adv_monitor.
> > >
> > > There are two options to resolve the build failures in our case.
> > > (1) Reorganize Makefile.am
> > > This option is to make the sources (except src/main.c) into a static
> > > lib and link this lib in bluetoothd executable target and whichever
> > > unit test the sources are required.
> > > (2) Create src/shared/am to facilitate helpers and core logic
> > > This option is to create a new source under src/shared/ to facilitate
> > > helper functions and core logic for src/adv_monitor. The interface of
> > > src/shared/am may have the following functions.
> > > - Create/destroy functions of struct adv_monitor
> > > - Create/destroy functions of struct adv_monitor_device
> > > - Helper function for monitor content matching.
> > > - Helper function for RSSI tracking
> > > However, the logic of RSSI tracking is hard to be ripped off and moved
> > > to src/shared/am. One example would be the use of timer in RSSI
> > > tracking, and there is currently no previous example of the timer use
> > > in the shared library.
> >
> > Not really following regarding the dependency on D-Bus, usually shared
> > don't have dependency on that because the code would be part of the
> > deamon and in that case you would be better of  testing that direct
> > via a tester that does exercise its D-Bus API. Lets be clear here,
> > except for gbus itself all our tests under unit directory are for C
> > function testing, if you want to test a D-Bus interface then we need a
> > tester that would do that over D-Bus.
> >
> > > Series https://patchwork.kernel.org/project/bluetooth/list/?series=351021
> > > is up for review. Our next step here would be ripped off the unit test
> > > for now and submit v5 of the series. Once we reach an conclusion on
> > > advmon unit test, we can submit a separate series including the
> > > refactoring and unit tests. Looking forward to any feedbacks. Thanks!
> >
> > I'd strip the testing for now since it doesn't seem we are on sync to
> > how we test things apparently.
> >
> Thanks for your reply. We need to rethink how to perform the testing.
> And agree, we do plan to strip the testing for now from our series.

Btw, I do recall seeing D-Bus level tests in chromium, have you guys
stopped developing them?

> > > Regards,
> > > Miao
> >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: Modifying Makefile.am to facilitate test-adv-monitor and future unit tests.

[Miao-chen Chou]

Hi Luiz,

On Thu, Sep 24, 2020 at 2:18 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Miao,
>
> On Thu, Sep 24, 2020 at 1:44 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> >
> > Hi Luiz,
> >
> >
> > On Mon, Sep 21, 2020 at 6:44 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Miao,
> > >
> > > On Mon, Sep 21, 2020 at 12:21 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> > > >
> > > > Hi Luiz,
> > > >
> > > > On Fri, Sep 18, 2020 at 3:12 PM Luiz Augusto von Dentz
> > > > <luiz.dentz@gmail.com> wrote:
> > > > >
> > > > > Hi Miao,
> > > > >
> > > > > On Fri, Sep 18, 2020 at 12:44 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> > > > > >
> > > > > > Hi Luiz and Marcel,
> > > > > >
> > > > > > Unlike the rest of the existing unit tests in BlueZ, the logic blocks
> > > > > > tested in test-adv-monitor require dependencies of not only
> > > > > > src/adv_monitor.c but also all the dependency tree of
> > > > > > src/adv_monitor.c. The current convention in Makefile.am is to add all
> > > > > > the extra dependencies one by one. However, the maintenance cost is
> > > > > > high and not suitable in the case of test-adv-monitor. Therefore, we'd
> > > > > > like to propose changes in Makefile.am to make the source of
> > > > > > bluetoothd as a static library and link it for bluetoothd target and
> > > > > > the unit test target. It would be great if you can provide feedback on
> > > > > > this idea before the implementation. Thanks in advance!
> > > > >
> > > > > Then we should have had the code move to src/shared for unit testing,
> > > > > but how exactly are you planning to do that? For testing the kernel
> > > > > interface it normally done via a dedicated tester, but that again can
> > > > > be done with shared library.
> > > > >
> > > > In series https://patchwork.kernel.org/project/bluetooth/list/?series=351021,
> > > > we introduced some helper functions in adv_monitor.h to perform unit
> > > > testing and test-adv-monitor to facilitate the unit tests of
> > > > adv_monitor. We are encountering an expected build check failure on
> > > > this series.
> > > >
> > > > There are two categories in test-adv-monitor, content filtering and
> > > > RSSI tracking, and content filter is easy to be moved to a standalone
> > > > shared component while RSSI tracking involves the use of timer, D-Bus
> > > > method calls and adv_monitor's internal structures, and that makes it
> > > > strongly coupled with the adv_monitor implementation which require a
> > > > tree of dependencies apart from adv_monitor.
> > > >
> > > > There are two options to resolve the build failures in our case.
> > > > (1) Reorganize Makefile.am
> > > > This option is to make the sources (except src/main.c) into a static
> > > > lib and link this lib in bluetoothd executable target and whichever
> > > > unit test the sources are required.
> > > > (2) Create src/shared/am to facilitate helpers and core logic
> > > > This option is to create a new source under src/shared/ to facilitate
> > > > helper functions and core logic for src/adv_monitor. The interface of
> > > > src/shared/am may have the following functions.
> > > > - Create/destroy functions of struct adv_monitor
> > > > - Create/destroy functions of struct adv_monitor_device
> > > > - Helper function for monitor content matching.
> > > > - Helper function for RSSI tracking
> > > > However, the logic of RSSI tracking is hard to be ripped off and moved
> > > > to src/shared/am. One example would be the use of timer in RSSI
> > > > tracking, and there is currently no previous example of the timer use
> > > > in the shared library.
> > >
> > > Not really following regarding the dependency on D-Bus, usually shared
> > > don't have dependency on that because the code would be part of the
> > > deamon and in that case you would be better of  testing that direct
> > > via a tester that does exercise its D-Bus API. Lets be clear here,
> > > except for gbus itself all our tests under unit directory are for C
> > > function testing, if you want to test a D-Bus interface then we need a
> > > tester that would do that over D-Bus.
> > >
> > > > Series https://patchwork.kernel.org/project/bluetooth/list/?series=351021
> > > > is up for review. Our next step here would be ripped off the unit test
> > > > for now and submit v5 of the series. Once we reach an conclusion on
> > > > advmon unit test, we can submit a separate series including the
> > > > refactoring and unit tests. Looking forward to any feedbacks. Thanks!
> > >
> > > I'd strip the testing for now since it doesn't seem we are on sync to
> > > how we test things apparently.
> > >
> > Thanks for your reply. We need to rethink how to perform the testing.
> > And agree, we do plan to strip the testing for now from our series.
>
> Btw, I do recall seeing D-Bus level tests in chromium, have you guys
> stopped developing them?
>
If you are referring to autotest, we are actively introducing new test
cases for both existing D-Bus interfaces and the newly-added
interfaces. In case of Adv monitor API, http://crrev.com/c/2384793 is
currently under review, and we have a test plan which includes
combination tests with suspend/resume, crashes, reconnection,
foreground scanning and functionalities.
> > > > Regards,
> > > > Miao
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Regards,
Miao