[Merge] lp:~renatofilho/mediaplayer-app/add-unittest into lp:mediaplayer-app

Jim Hodapp jim.hodapp at canonical.com
Mon Sep 26 18:48:59 UTC 2016


Review: Needs Fixing code

A few comments inline. Also please remove the following two lines from debian/control:

gstreamer1.0-hybris [armhf],
gstreamer0.10-plugins-good [amd64 i386],

Diff comments:

> 
> === modified file 'debian/control'
> --- debian/control	2016-09-21 15:27:02 +0000
> +++ debian/control	2016-09-26 18:13:50 +0000
> @@ -12,6 +12,12 @@
>                 qtbase5-dev,
>                 qtdeclarative5-dev,
>                 qtmultimedia5-dev,
> +# used by unittest

I'd align this comment's indentation with the line above/below it

> +               qml-module-qtmultimedia | qml-module-qtmultimedia-gles,
> +               qml-module-qttest [amd64 armhf i386],

Also consider adding arm64 to all of these.

> +               qml-module-ubuntu-test [amd64 armhf i386],
> +               qtdeclarative5-dev-tools [amd64 armhf i386],
> +               xvfb [amd64 armhf i386],
>  Standards-Version: 3.9.5
>  # If you don't have commit rights to lp:mediaplayer-app but need to upload
>  # packaging changes, just go ahead.  The developers will notice and sync
> 
> === modified file 'tests/unittest/CMakeLists.txt'
> --- tests/unittest/CMakeLists.txt	2013-04-04 21:22:18 +0000
> +++ tests/unittest/CMakeLists.txt	2016-09-26 18:13:50 +0000
> @@ -13,18 +13,50 @@
>                 ${CMAKE_CURRENT_BINARY_DIR}/test-config.h)
>  
>  # thumbnail-test ##############################################################
> -add_executable(thumbnailtest
> -               thumbnailtest.cpp
> -               ${mediaplayer_src_SOURCE_DIR}/thumbnail-pipeline-gst.cpp
> -)
> +#add_executable(thumbnailtest

Get rid of this reference to the old unit test. I tried getting this to work and I don't think it ever truly worked. It's captured by bzr commits, so no need to clutter current code with it.

> +#               thumbnailtest.cpp
> +#               ${mediaplayer_src_SOURCE_DIR}/thumbnail-pipeline-gst.cpp
> +#)
>  
> -qt5_use_modules(thumbnailtest Gui Core Test)
> -add_test(thumbnailtest thumbnailtest -xunitxml -o test_thumbnailtest.xml)
> -set_tests_properties(thumbnailtest PROPERTIES
> -                     TIMEOUT ${CTEST_TESTING_TIMEOUT}
> -                     ENVIRONMENT "QT_QPA_PLATFORM=minimal")
> -target_link_libraries(thumbnailtest 
> -                      ${GSTLIB_LDFLAGS})
> +#qt5_use_modules(thumbnailtest Gui Core Test)

Get rid of this reference to the old unit test. I tried getting this to work and I don't think it ever truly worked. It's captured by bzr commits, so no need to clutter current code with it.

> +#add_test(thumbnailtest thumbnailtest -xunitxml -o test_thumbnailtest.xml)
> +#set_tests_properties(thumbnailtest PROPERTIES
> +#                     TIMEOUT ${CTEST_TESTING_TIMEOUT}
> +#                     ENVIRONMENT "QT_QPA_PLATFORM=minimal")
> +#target_link_libraries(thumbnailtest
> +#                      ${GSTLIB_LDFLAGS})
>  ###############################################################################
>  
>  
> +find_program(QMLTESTRUNNER_BIN
> +    NAMES qmltestrunner
> +    PATHS /usr/lib/*/qt5/bin
> +    NO_DEFAULT_PATH
> +)
> +
> +find_program(XVFB_RUN_BIN
> +    NAMES xvfb-run
> +)
> +
> +macro(DECLARE_QML_TEST TST_NAME TST_QML_FILE)
> +    if(USE_XVFB)
> +        set(COMMAND_PREFIX ${XVFB_RUN_BIN} -a -s "-screen 0 1024x768x24")
> +    else()
> +        set(COMMAND_PREFIX "")
> +    endif()
> +    add_test(NAME ${TST_NAME}
> +        WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
> +        COMMAND ${COMMAND_PREFIX} ${QMLTESTRUNNER_BIN} -input ${CMAKE_CURRENT_SOURCE_DIR}/${TST_QML_FILE}
> +    )
> +endmacro()
> +
> +if(QMLTESTRUNNER_BIN AND XVFB_RUN_BIN)
> +    declare_qml_test("video_player" tst_video_player.qml)
> +else()
> +    if (NOT QMLTESTRUNNER_BIN)
> +        message(WARNING "Qml tests disabled: qmltestrunner not found")
> +    else()
> +        message(WARNING "Qml tests disabled: xvfb-run not found")
> +    endif()
> +endif()
> +


-- 
https://code.launchpad.net/~renatofilho/mediaplayer-app/add-unittest/+merge/306784
Your team Ubuntu Phablet Team is subscribed to branch lp:mediaplayer-app.



More information about the Ubuntu-reviews mailing list