Merge lp://staging/~adam-disc0tech/ubuntu-autopilot-tests/fileroller into lp://staging/ubuntu-autopilot-tests

Proposed by Adam Smith
Status: Needs review
Proposed branch: lp://staging/~adam-disc0tech/ubuntu-autopilot-tests/fileroller
Merge into: lp://staging/ubuntu-autopilot-tests
Diff against target: 149 lines (+53/-17)
2 files modified
ubuntu_autopilot_tests/fileroller/__init__.py (+0/-1)
ubuntu_autopilot_tests/fileroller/test_fileroller.py (+53/-16)
To merge this branch: bzr merge lp://staging/~adam-disc0tech/ubuntu-autopilot-tests/fileroller
Reviewer Review Type Date Requested Status
Nicholas Skaggs (community) Approve
Chris Gagnon Pending
Dan Chapman  Pending
Review via email: mp+211072@code.staging.launchpad.net

This proposal supersedes a proposal from 2014-02-25.

Description of the change

Fixes https://bugs.launchpad.net/ubuntu-autopilot-tests/+bug/1206386 by doing two things

1) correcting the way content is selected for the archive
2) checking the correct content is extracted
3) home directory is now mocked

To post a comment you must log in.
Revision history for this message
Nicholas Skaggs (nskaggs) wrote : Posted in a previous version of this proposal

Thanks for the merge Adam. I have a few questions;

We can't use sudo in a test; this are non-interactive, and I'm confused why you would need root anyway. However, the symbolic linking to /home is very confusing to me; what are you trying to accomplish?

Adding logging and the 'check the files have actually been extracted' are both welcome additions.

review: Needs Fixing
Revision history for this message
Adam Smith (adam-disc0tech) wrote : Posted in a previous version of this proposal

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Hi

You are right, sudo shouldn't even be required.

What I'm trying to achieve, is to create a syncing in home with the name ___000, pointing to the content under /usr/share/example-content.

This seemed easier / safer to me than navigating the folder structure through the gui. This is the bit that seemed to be broken when I picked up the test. I can be pretty sure that the link is the first item listed in home.

- -adam

On 25 February 2014 21:30:51 GMT+00:00, Nicholas Skaggs <email address hidden> wrote:
>
>Review: Needs Fixing
>
>Thanks for the merge Adam. I have a few questions;
>
>We can't use sudo in a test; this are non-interactive, and I'm confused
>why you would need root anyway. However, the symbolic linking to /home
>is very confusing to me; what are you trying to accomplish?
>
>Adding logging and the 'check the files have actually been extracted'
>are both welcome additions.
>--
>https://code.launchpad.net/~adam-disc0tech/ubuntu-autopilot-tests/fileroller/+merge/208183
>You are the owner of
>lp:~adam-disc0tech/ubuntu-autopilot-tests/fileroller.

- --
Sent from my Android device with K-9 Mail. Please excuse my brevity.
-----BEGIN PGP SIGNATURE-----
Version: APG v1.0.9

iQI8BAEBCAAmBQJTDaQrHxxkaXNjMHRlY2ggPGFkYW1AZGlzYzB0ZWNoLmNvbT4A
CgkQveW9bcQ9QGwjtRAAiPgywHrGVnfnwYm2pxIEeYZHjbXQp2Y2UQ2WMebuB8iM
vvPDPxm6tPudxZ63QFooUNTS7Y3FiAfnyhLSDM93vCeIfysTBVZ6Va9WuT02B8Is
SyGaNLI4pV0PL4c/xNkK2FPWjJtL62W6Q9WxmYLjbi6jw+tKA6aa87QIsO6aioW/
2FgF3wtyMU24iYkNSStRls50EYiG9BflmYobWagAolpJ1ZfQEW6Os+nVMaaJULOY
ow9CLSitwm4huSNEhFk8pPb2QFH5T+YEhfIcXZA5OGOLJGbQ3uWE+2kAkDOlPSfk
hSUIREMcaqPIoW3/EidbOCU24cRUrl9Q901s6xYGBU1ql7wa7mbIFdNhqZk0hnJj
R/6QrN+LF14s0GeO/+GA4Jts+LTLhNHaxUau9W9Yq+r3nbLV9qs9//jW5+1TjUPF
31kF8PIol1AEehEqVtVCdOreEEd0dVvOaY/gX2K0LrkWY8WEBMPf9Rb8hDf5SoEe
XO/QzjUcSaobPNA3iNyS9aX4yprGfuVONjcTE3qkwZk17X4heA5EReVeA/8Nb2d1
83Ga7EqDCkN7IlhCdDufwM+od7d6lIHNmkucDSghwjPRgSdo/jZaaUJGT0UvOI0W
oZmuH9ZHp39EjrGhoaQncru/3X24WfZBs+gfwpsbwPXBWkdcVBZIFnTr1YTQCdo=
=lWOT
-----END PGP SIGNATURE-----

Revision history for this message
Adam Smith (adam-disc0tech) wrote : Posted in a previous version of this proposal

Fixed

Revision history for this message
Nicholas Skaggs (nskaggs) wrote : Posted in a previous version of this proposal

I'm still not sure on using ___000 in home. Let me suggest something we can both approve of; mocking /home and doing the same thing (though you can pick a saner name as your folder will be the only one in the new "/home"). This will ensure we don't leave any remenants of testing behind, nor mangle with /home dirs. Be sure and put the mock location as a temporary dir in /tmp.

You can do this with python mock. For an example see http://bazaar.launchpad.net/~music-app-dev/music-app/trunk/view/head:/tests/autopilot/music_app/tests/__init__.py, specifically in regards to _patch_home.

review: Needs Fixing
Revision history for this message
Chris Gagnon (chris.gagnon) wrote : Posted in a previous version of this proposal

I saw this in my email and thought I should comment.

import's should not be on the same line according to pep8 http://legacy.python.org/dev/peps/pep-0008/#imports, it's also good practice to keep them in alphabetical order.

13 +import os, subprocess
14 +import shutil, logging
15 import tempfile

should be:

import logging
import os
import shutil
import subprocess
import tempfile

if you apt-get install python-flake8 you can run 'flake8 some_file.py' and it will tell you about the pep8 style guides problems in your code.

A lot of code editors have a flake8 plugin that will tell you when you have errors, so you don't have to do it manually.

review: Needs Fixing
Revision history for this message
Adam Smith (adam-disc0tech) wrote :

All comments above taken into account, PEP8 compliance done, home directory mocked, ___000 references removed, no more symlink...

72. By Adam Smith

removed redundant method

73. By Adam Smith

Removed 25 character temp directory code, not required for this app test

Revision history for this message
Nicholas Skaggs (nskaggs) wrote :

BTW, copying the XAuthority file is for jenkins, so i suppose we can retain it for now. I approve the clean-up, it's a step in the right direction. Any chance for removing the sleeps and checking for status instead?

review: Approve

Unmerged revisions

73. By Adam Smith

Removed 25 character temp directory code, not required for this app test

72. By Adam Smith

removed redundant method

71. By Adam Smith

Implemented fake home directory

70. By Adam Smith

Syntax issue fixed

69. By Adam Smith

PEP8 compliance issues (mostly) fixed

68. By Adam Smith

Removed unnecessary use of root

67. By Adam Smith

Hit UP a lot when you enter the file dialog to ensure the same starting location

66. By Adam Smith

Added to step to go back to home directory each time the file dialog is opened

65. By Adam Smith

Changed mechanism for identifying content to archive.

64. By Adam Smith

Added assertions to check files and directories are actually extracted

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches