ubuntu-touch-coreapps-reviewers team mailing list archive

[Andrew Hayzen <ahayzen@xxxxxxxxx>]

Review: Needs Fixing

Some more inline comments :-)

1) I don't think you need to add the visible: false as the object is created when it is required.
2) I wonder if this method should be get_library_empty_state() or get_library_empty_state_page() ? To match the pattern of the other methods?
3) Can the double space be fixed to a single space?
4) This import can be removed, see further details below.
5) If the first os.remove() fails, then this throws the exception before it can attempt to remove the others?
6) Change this to
except Exception as e:
    logger.debug("Mediscanner patching failed %s" % e)

Or for exactly the same
except Exception as e:
    logger.debug("Mediscanner patching failed %s" % type(e))

Diff comments:

> === modified file 'app/ui/LibraryEmptyState.qml'
> --- app/ui/LibraryEmptyState.qml	2015-10-28 01:05:33 +0000
> +++ app/ui/LibraryEmptyState.qml	2015-12-19 01:21:56 +0000
> @@ -22,6 +22,8 @@
>  
>  Page {
>      id: libraryEmptyPage
> +    objectName: "emptyLibrary"
> +    visible: false

I don't think you need to add the visible: false as the object is created when it is required.

>      anchors {
>          fill: parent
>      }
> 
> === modified file 'tests/autopilot/music_app/__init__.py'
> --- tests/autopilot/music_app/__init__.py	2015-11-03 03:55:07 +0000
> +++ tests/autopilot/music_app/__init__.py	2015-12-19 01:21:56 +0000
> @@ -135,6 +135,16 @@
>          # wait for now playing page to be visible
>          self.get_now_playing_page().visible.wait_for(True)
>  
> +    def get_LibraryEmptyState(self):

I wonder if this method should be get_library_empty_state() or get_library_empty_state_page() ? To match the pattern of the other methods?

> +        return self.app.wait_select_single(LibraryEmptyState,
> +                                           objectName="emptyLibrary")
> +
> +
> +class LibraryEmptyState(UbuntuUIToolkitCustomProxyObjectBase):
> +    """Autopilot helper for LibraryEmptyState"""
> +    def __init__(self, *args):
> +        super(LibraryEmptyState, self).__init__(*args)
> +
>  
>  class Page(UbuntuUIToolkitCustomProxyObjectBase):
>      """Autopilot helper for Pages."""
> 
> === modified file 'tests/autopilot/music_app/content/mediascanner-2.0/mediastore.sql'
> --- tests/autopilot/music_app/content/mediascanner-2.0/mediastore.sql	2015-11-03 02:43:10 +0000
> +++ tests/autopilot/music_app/content/mediascanner-2.0/mediastore.sql	2015-12-19 01:21:56 +0000
> @@ -2,7 +2,7 @@
>  DELETE FROM `schemaVersion`;
>  INSERT INTO `schemaVersion` VALUES(10);
>  
> -DROP TABLE media;
> +DROP TABLE  media;

Can the double space be fixed to a single space?

>  CREATE TABLE media (
>      filename TEXT PRIMARY KEY NOT NULL CHECK (filename LIKE '/%'),
>      content_type TEXT,
> 
> === added directory 'tests/autopilot/music_app/content/mediascanner-2.0/songs'
> === renamed file 'tests/autopilot/music_app/content/1.ogg' => 'tests/autopilot/music_app/content/mediascanner-2.0/songs/1.ogg'
> === renamed file 'tests/autopilot/music_app/content/2.ogg' => 'tests/autopilot/music_app/content/mediascanner-2.0/songs/2.ogg'
> === renamed file 'tests/autopilot/music_app/content/3.mp3' => 'tests/autopilot/music_app/content/mediascanner-2.0/songs/3.mp3'
> === modified file 'tests/autopilot/music_app/tests/__init__.py'
> --- tests/autopilot/music_app/tests/__init__.py	2015-02-03 14:42:34 +0000
> +++ tests/autopilot/music_app/tests/__init__.py	2015-12-19 01:21:56 +0000
> @@ -22,6 +22,7 @@
>  import sqlite3
>  import logging
>  import music_app
> +import sys

This import can be removed, see further details below.

>  
>  import fixtures
>  from music_app import MusicApp
> @@ -141,29 +132,40 @@
>              logger.debug("Patched home to fake home directory %s" % temp_dir)
>          return temp_dir
>  
> -    def _create_music_library(self):
> +    def _create_music_library(self, db_dir):
>          logger.debug("Creating music library for %s test" % self.test_type)
>          logger.debug("Home set to %s" % self.home_dir)
>          musicpath = os.path.join(self.home_dir, 'Music')
>          logger.debug("Music path set to %s" % musicpath)
>          mediascannerpath = os.path.join(self.home_dir,
>                                          '.cache/mediascanner-2.0')
> +        logger.debug("Mediascanner path set to %s" % mediascannerpath)
> +
>          if not os.path.exists(musicpath):
>              os.makedirs(musicpath)
> -        logger.debug("Mediascanner path set to %s" % mediascannerpath)
>  
>          # set content path
>          content_dir = os.path.join(os.path.dirname(music_app.__file__),
> -                                   'content')
> -
> +                                   'content', db_dir)
> +        songs_dir = os.path.join(content_dir, 'songs')
>          logger.debug("Content dir set to %s" % content_dir)
>  
>          # copy content
> -        shutil.copy(os.path.join(content_dir, '1.ogg'), musicpath)
> -        shutil.copy(os.path.join(content_dir, '2.ogg'), musicpath)
> -        shutil.copy(os.path.join(content_dir, '3.mp3'), musicpath)
> -
> -        logger.debug("Music copied, files " + str(os.listdir(musicpath)))
> +        if os.path.isdir(songs_dir) and db_dir == 'mediascanner-2.0':
> +            shutil.copy(os.path.join(songs_dir, '1.ogg'), musicpath)
> +            shutil.copy(os.path.join(songs_dir, '2.ogg'), musicpath)
> +            shutil.copy(os.path.join(songs_dir, '3.mp3'), musicpath)
> +
> +            logger.debug("Music copied, files " + str(os.listdir(musicpath)))
> +        elif not os.path.isdir(songs_dir):
> +            try:
> +                os.remove(os.path.join(songs_dir, '1.ogg'))

If the first os.remove() fails, then this throws the exception before it can attempt to remove the others?

> +                os.remove(os.path.join(songs_dir, '2.ogg'))
> +                os.remove(os.path.join(songs_dir, '3.mp3'))
> +            except OSError:
> +                pass
> +
> +            logger.debug("Music deleted, files " + str(os.listdir(musicpath)))
>  
>          if self.test_type is not 'click':
>              self._patch_mediascanner_home(content_dir, mediascannerpath)
> @@ -190,7 +193,12 @@
>          f = open(mediascannerpath + "/mediastore.sql", 'rb')
>          sql = f.read().decode("utf-8")
>          cur = con.cursor()
> -        cur.executescript(sql)
> +        try:
> +            cur.executescript(sql)
> +            con.commit()
> +        except:
> +            logger.debug("Mediscanner patching failed %s" % sys.exc_info()[0])

Change this to
except Exception as e:
    logger.debug("Mediscanner patching failed %s" % e)

Or for exactly the same
except Exception as e:
    logger.debug("Mediscanner patching failed %s" % type(e))

> +            raise
>          con.close()
>  
>          logger.debug(


-- 
https://code.launchpad.net/~vamrocks602/music-app/test-empty-library/+merge/280794
Your team Music App Developers is subscribed to branch lp:music-app.