Merge lp://qastaging/~canonical-platform-qa/camera-app/fix_base_class into lp://qastaging/camera-app

Proposed by Leo Arias
Status: Merged
Approved by: Florian Boucault
Approved revision: 555
Merged at revision: 557
Proposed branch: lp://qastaging/~canonical-platform-qa/camera-app/fix_base_class
Merge into: lp://qastaging/camera-app
Diff against target: 121 lines (+11/-43)
3 files modified
tests/autopilot/camera_app/emulators/baseemulator.py (+0/-35)
tests/autopilot/camera_app/emulators/panel.py (+2/-2)
tests/autopilot/camera_app/tests/__init__.py (+9/-6)
To merge this branch: bzr merge lp://qastaging/~canonical-platform-qa/camera-app/fix_base_class
Reviewer Review Type Date Requested Status
Brendan Donegan (community) Approve
PS Jenkins bot continuous-integration Needs Fixing
Ubuntu Phablet Team Pending
Review via email: mp+257658@code.qastaging.launchpad.net

Commit message

Use the base class from the toolkit in autopilot tests.

Description of the change

Are there any related MPs required for this MP to build/function as expected? Please list.

No related MPs.

Is your branch in sync with latest trunk (e.g. bzr pull lp:trunk -> no changes)

Yes it is.

Did you perform an exploratory manual test run of your code change and any related functionality on device or emulator?

Only made sure the automated tests were working as before.

Did you successfully run all tests found in your component's Test Plan (https://wiki.ubuntu.com/Process/Merges/TestPlan/<package-name>) on device or emulator?

Only changes to the automated tests, so no need to run the test plan.

If you changed the UI, was the change specified/approved by design?

No UI changes.

If you changed UI labels, did you update the pot file?

No UI changes.

If you changed the packaging (debian), did you add a core-dev as a reviewer to this MP?

No debian changes.

Autopilot is currently designed to have a single proxy object base, so we started finding some problems when we integrated tests from multiple projects for the sanity suite. We are working on making autopilot smarter, but in order to be able to change the design without breaking any tests we need this change.
And in this case, it makes sense to use the proxy object from the toolkit, because the one defined for this project was just duplicating the code.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Christopher Lee (veebers) wrote :

Looks good to me, although I will moan that there are changes here that aren't directly relevant to the base CPO class change (i.e. removal of unused imports).

The CI failed due to an infrastructure timeout, firing off again hopefully we have better luck this time around.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Brendan Donegan (brendan-donegan) wrote :

I'm inclined to agree with veebers - single issue changes please to avoid the chance of unrelated things blocking the merge approval (see how it is already :P). Also the mix of specifying the base class as a variable and also directly is a little jarring - couldn't you make it a class attribute or 'constant' (i.e. PROXY_BASE = ubuntuuitoolkit.UbuntuUIToolkitCustomProxyObjectBase at the file level)

review: Needs Information
554. By Leo Arias

Reverted the unused imports, will remove them in a following branch.

Revision history for this message
Leo Arias (elopio) wrote :

> Looks good to me, although I will moan that there are changes here that aren't
> directly relevant to the base CPO class change (i.e. removal of unused
> imports).
>
> The CI failed due to an infrastructure timeout, firing off again hopefully we
> have better luck this time around.

Right... we have discussed about it and I keep doing it. Sorry.
Pushed.

555. By Leo Arias

Use a constant for the base class.

Revision history for this message
Leo Arias (elopio) wrote :

> Also the mix of specifying the base class as a variable and also directly
> is a little jarring - couldn't you make it a class attribute or 'constant'
> (i.e. PROXY_BASE = ubuntuuitoolkit.UbuntuUIToolkitCustomProxyObjectBase at the
> file level)

That's an alias, because the name is too big to fit the 88 columns.
A constant works well too, so pushed.
Maybe we need to add a shorter alias directly on the toolkit.

Revision history for this message
Max Brustkern (nuclearbob) wrote :

This seems reasonable to me once the CI jobs stop failing.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Brendan Donegan (brendan-donegan) wrote :

Looks good to me

review: Approve

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