Merge lp://staging/~fazerlicourice/music-app/test-empty-library into lp://staging/music-app

In addition to the comments below, does this pass pep8?

So add the removal code, and this is a +1 from me. Good work!

Looking good so far, a few inline comments, I haven't run the code yet these are just observations :-)

Also the code should pass PEP8 and pyflakes, see the link [0] for the current errors.

And could you add an entry for yourself in the debian/changelog (simply run $ dch) and link bug 1261587 as has been done in other entries.

0 - http://pastebin.ubuntu.com/14082325/

I don't think we should be setting the schema version of mediascanner2 back to version 9. If it is perhaps an issue for developers/testers not running the development release, then we need to add instructions on how to install the newest release of mediascanner2 that is on the phone. Likewise, I think the other changes to the mediastore.db file should be reverted.

It now passes pep8 and snowflake tests and I have added an entry for me in debian/changelog.

If I attempt to edit any of the .sql files, when I try to read them and convert them to .db, it doesn't work. The current .db files are from nskaggs' patch. I was unable to get the sql stuff to work. I get the error- 'no such table: schemaVersion' just like I was getting earlier. If I revert back to the patch that nskaggs had provided, they all work, but if I make any changes and then attempt to read them with "sqlite3 mediastore.db < mediastore.sql" it doesn't work.

Victor, ohh you DO want schema 10? I needed it locally, but for some reason, I thought you wanted schema 9. The changes to mediastore.db should be easy enough to revert if you don't like / want them.

Vamshi, The new files are in tests/autopilot/music_app/content/blank-mediascanner-2.0. You don't need to touch them and they are fine. I repeat, you don't need to create any database files, so don't worry about that.

From reading the comments I would:

1) Update the insert schema version to 10 in the sql files
2) Checkout the older revision of tests/autopilot/music_app/content/mediascanner-2.0/mediastore.db and replace it so there is no diff with this MP.

One additional comment on how you are removing files.

Like I said earlier, when I try to edit the sql files, the tests fail regardless of whether or not I had sqlite3 read them and convert to .db. Initially I'll get an error that says incomplete sql, this is because there is no semi-colon at the end of the line. After I add the semi-colon, it says no such table schemaVersion.

I changed the schemaVersion to 10 and I changed the file removal parameter. The tests no longer work for me because its schemaVersion 10 but Nicholas informed me that its right.

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

Andrew, I made those changes and pushed.

When I run these tests on the device or when run on the desktop, each test produces the "No music found" view. Here is the output on the desktop: http://paste.ubuntu.com/14123722/

The error is "Could not initialise media store: Tried to open a db with schema version 9, while supported version is 10."

bzr thinks that the mediastore.db has been modified, so what changes have been made to that?

=== modified file 'tests/autopilot/music_app/content/mediascanner-2.0/mediastore.db'
Binary files tests/autopilot/music_app/content/mediascanner-2.0/mediastore.db 2015-11-01 16:17:33 +0000 and tests/autopilot/music_app/content/mediascanner-2.0/mediastore.db 2015-12-20 00:11:18 +0000 differ

I didn't make any changes to the .db files. All I did was change the
schemaVersion in the .sql files to 10. I tried converting those .sql files
to .db files but I got some errors and when I told Nicholas he said that I
don't need to do that, and that I should leave the .db files as they are.

Vamshi Balanaga

On Mon, Dec 21, 2015 at 7:21 AM, Andrew Hayzen <email address hidden> wrote:

> The error is "Could not initialise media store: Tried to open a db with
> schema version 9, while supported version is 10."
>
> bzr thinks that the mediastore.db has been modified, so what changes have
> been made to that?
>
> === modified file
> 'tests/autopilot/music_app/content/mediascanner-2.0/mediastore.db'
> Binary files
> tests/autopilot/music_app/content/mediascanner-2.0/mediastore.db 2015-11-01
> 16:17:33 +0000 and
> tests/autopilot/music_app/content/mediascanner-2.0/mediastore.db 2015-12-20
> 00:11:18 +0000 differ
> --
>
> https://code.launchpad.net/~vamrocks602/music-app/test-empty-library/+merge/280794
> You are the owner of lp:~vamrocks602/music-app/test-empty-library.
>

Vamshi,

It seems that the test you added is failing: http://paste.ubuntu.com/14134202/

Should "get_LibraryEmptyState" actually be "get_library_empty_state_page" or something else perhaps?

I apologize for that, I thought that I changed that. It's done now, just
pushed.

Vamshi,

The test still does not work. Here's the log output--I believe you need to ad "_page" to the method call.

test-log: {{{
18:34:56.811 INFO logging:45 - TestEmptyLibrary: launch_test_local. Arguments (). Keyword arguments: {}.
18:34:56.811 WARNING base:52 - This function is deprecated. Use get_toolkit_launcher_command() instead.
18:34:56.811 INFO _launcher:392 - Attempting to launch application '/usr/lib/x86_64-linux-gnu/qt5/bin/qmlscene' with arguments '/home/victor/Development/test-empty-library/app/music-app.qml debug' as a normal process
18:34:56.821 INFO _launcher:450 - Launching process: ['/usr/lib/x86_64-linux-gnu/qt5/bin/qmlscene', '-testability', '/home/victor/Development/test-empty-library/app/music-app.qml', 'debug']
18:35:00.423 INFO _launcher:563 - waiting for process to exit.
18:35:00.423 INFO _launcher:586 - Killing process 36773
18:35:00.531 WARNING content:83 - Followed stream is empty.
}}}

Traceback (most recent call last):
  File "/home/victor/Development/test-empty-library/tests/autopilot/music_app/tests/test_music.py", line 32, in test_display_message_when_no_music
    library = self.app.get_library_empty_state()
AttributeError: 'MusicApp' object has no attribute 'get_library_empty_state'

Ran 20 tests in 253.585s
FAILED (failures=1)

Also, you'll need to resolve the merge conflict in the debian/changelog file. We get such conflicts frequently as multiple people work to propose merges.

To fix, do the following:

$ bzr merge lp:music-app

Then, fix the conflict in the debian/changelog and save it. Then run:

$ bzr resolve debian/changelog

From there just commit and push the changes as usual.

Lastly, when I run this on the device I get the "No music found" page consistently. I imagine there might be something in the change to how we do mocking that might have caused it.

ahazyen, balloons: Can you guys work with Vamshi to fix this issue?

I fixed the method name, and resolved the conflict.

Vamshi Balanaga

On Tue, Dec 22, 2015 at 8:09 PM, Victor Thompson <email address hidden>
wrote:

> Review: Needs Fixing
>
> Lastly, when I run this on the device I get the "No music found" page
> consistently. I imagine there might be something in the change to how we do
> mocking that might have caused it.
>
> ahazyen, balloons: Can you guys work with Vamshi to fix this issue?
> --
>
> https://code.launchpad.net/~vamrocks602/music-app/test-empty-library/+merge/280794
> You are the owner of lp:~vamrocks602/music-app/test-empty-library.
>

Thanks Vamshi.

We still need input/verification on the device tests. I won't have the time to help you with this in the near term.

By verification, do you mean you have to see if it works? Or do know that it doesn't work and you need to see what's wrong?

Hey Vamshi! I've asked Andrew to do 2 things.

1) Test this out so we can merge it :-)
2) Add some more autopilot tasks to GCI so you can have some more fun.

Thanks again for working on this and sorry for the delay in verifying.

Thank you very much.

OK this is erroring still due to
[Errno 2] No such file or directory: '/tmp/adt-run.XH20gO/tree/tests/autopilot/music_app/content/blank-mediascanner-2.0/songs/1.ogg'
[Errno 2] No such file or directory: '/tmp/adt-run.XH20gO/tree/tests/autopilot/music_app/content/blank-mediascanner-2.0/songs/2.ogg'
[Errno 2] No such file or directory: '/tmp/adt-run.XH20gO/tree/tests/autopilot/music_app/content/blank-mediascanner-2.0/songs/3.mp3'

However it was silently failing, so we didn't know it was unable to remove the files, full log with my extra debugging can be found here [0]

There are two inline comments (3 instances of each) to fix, which should resolve this
1) This should be
os.remove(os.path.join(musicpath, '1.ogg'))
2) As these were failing silently, can we change these to
except OSError as e:
    logger.debug("Error removing" + str(e))

Once you have fixed those issues, the test passes for me :-) My diff looks like this [1]

0 - http://pastebin.ubuntu.com/14404744/
1 - http://pastebin.ubuntu.com/14404783/

Made the changes and pushed.

LGTM! Thanks for your patience and time spent on this :-)

Took me 4 runs but I eventually got

test autopilot: - - - - - - - - - - results - - - - - - - - - -
autopilot PASS

\o/

For the other 3 runs I had test_shuffle (once) and test_pressing_prev_from_first_song_plays_last_when_repeat_on (twice) fail looks like they are possibly flaky tests, hopefully test_shuffle will be improved after the bgplaylists, or just because I'm running custom versions of qtubuntu-media.

Running from the deksopt:

Tests running...

Ran 20 tests in 392.937s
OK

We can try to fix the flakiness of the tests at a later time--as it wouldn't have been introduced under this change.

Thanks! lgtm!

FAILED: Autolanding.
More details in the following jenkins job:
https://core-apps-jenkins.ubuntu.com/job/music-app-autolanding/8/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-land-mp/1393/console

FAILED: Autolanding.
Approved revid is not set in launchpad. This is most likely a launchpad issue and re-approve should fix it. There is also a chance (although a very small one) this is a permission problem of the ps-jenkins bot.
https://core-apps-jenkins.ubuntu.com/job/music-app-autolanding/9/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-land-mp/1394/console