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

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

This proposal has been superseded by a proposal from 2014-03-14.

Commit message

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

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

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

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 :

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

68. By Adam Smith

Removed unnecessary use of root

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

Fixed

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

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 :

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
69. By Adam Smith

PEP8 compliance issues (mostly) fixed

70. By Adam Smith

Syntax issue fixed

71. By Adam Smith

Implemented fake home directory

72. By Adam Smith

removed redundant method

73. By Adam Smith

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

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