Merge into main : configs : lp:~sylvain-pineau/checkbox/+git/metabox : Git : Code : Checkbox

Status:	Merged
Approved by:	Sylvain Pineau on 2021-03-31
Approved revision:	eef9b23a625e4b2028ba23a4e0b7c819d4a25507
Merged at revision:	dcca97f373be96584de91512bd64c26453d704f1
Proposed branch:	~sylvain-pineau/checkbox/+git/metabox:configs
Merge into:	~checkbox-dev/checkbox/+git/metabox:main
Diff against target:	2750 lines (+2538/-5) 31 files modified .gitignore (+6/-0) MANIFEST.in (+2/-0) configs/checkbox-core-snap-classic-beta-config.py (+20/-0) configs/dev-ppa-config.py (+17/-0) configs/testing-ppa-config.py (+17/-0) metabox/__init__.py (+0/-0) metabox/core/__init__.py (+0/-0) metabox/core/actions.py (+97/-0) metabox/core/aggregator.py (+50/-0) metabox/core/configuration.py (+144/-0) metabox/core/keys.py (+36/-0) metabox/core/lxd_execute.py (+236/-0) metabox/core/lxd_provider.py (+229/-0) metabox/core/machine.py (+433/-0) metabox/core/runner.py (+221/-0) metabox/core/scenario.py (+244/-0) metabox/core/utils.py (+49/-0) metabox/lxd_profiles/checkbox.profile (+35/-0) metabox/lxd_profiles/lowmem.profile (+3/-0) metabox/lxd_profiles/snap.profile (+24/-0) metabox/main.py (+65/-1) metabox/metabox-provider/manage.py (+9/-0) metabox/metabox-provider/units/basic-jobs.pxu (+224/-0) metabox/metabox-provider/units/basic-tps.pxu (+39/-0) metabox/scenarios/__init__.py (+0/-0) metabox/scenarios/basic/__init__.py (+0/-0) metabox/scenarios/basic/run-invocation.py (+86/-0) metabox/scenarios/desktop_env/launcher.py (+76/-0) metabox/scenarios/restart/launcher.py (+46/-0) metabox/scenarios/urwid/run-invocation.py (+111/-0) setup.py (+19/-4)
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Maciej Kisielewski (community)	2021-03-29	Approve on 2021-03-31
Sylvain Pineau (community)		Needs Resubmitting on 2021-03-30
Review via email: mp+400333@code.qastaging.launchpad.net

Description of the change

First commit (configs now define an URI when they are referring to a file/dir path or a ppa isntead of abusing the origin field)

Revision history for this message

Maciej Kisielewski (kissiel) wrote on 2021-03-29:

#

2.5kLOC dump is difficult to review. This is why I started with just configs/cli opts. Do you have a version with a better split? Like a more granular git history or something.

Also one quick comment inline.

review: Needs Information

Revision history for this message

Sylvain Pineau (sylvain-pineau) wrote on 2021-03-29:

#

I can drop comments in the config.py indeed or even prepare some production files, one for ppa, one for checkbox-core snaps

Here's the unsquashed version of the history: https://code.launchpad.net/~sylvain-pineau/checkbox/+git/metabox/+merge/400341 (not super clean though)

Revision history for this message

Maciej Kisielewski (kissiel) wrote on 2021-03-30:

#

Blocking problems:
I'd like to ask for a cleanup of physical structure of scenarios.
As in reboot is not basic, neither is audio/opengl stuff.
The configs cleanup.

./setup test fails, no dependency on yaml declared.

Non-blocking issues (Don't have to be resolved at this time):

It'd be nicer if yaml files had a '.yaml' suffix.

In the scenarios I'd not match against fancy unicode (checkmark). Does remote even print that glyph?

In the steps:
- Send is a misnomer. Send signal? Send file? It's for typing keys, so TBH, it should just be PushKeys, Type, or soemthing that's close to reality.
For the keys themselves, I think having to use keys.KEY_SPACE for ' ' is bad. The spaces from the string should get auto translated into that. Same goes for enter (\n or \r).

I still think that no audio or rendering should happen. It's just against test design principles. If checkbox is responsible for forwarding some environment info that's needed for audio tests then we should test for those variables, not variables _and_ actual hardware. This frees up dependencies, time, etc.

I still think that assertions are not actions. Assertions should be checked against completed scenario. Like assertion on output or a report. The action is expect, where indeed there is a sequence of things that happen.

The setup commands run in the provider are worse than what we originally had. Less readable, with more escaping and bashisms.

Encoding re patterns to bytes and running searches on bytes is backwards. The output that's being searched should be decoded using stdout's encoding. Instead of hardcoding the pattern encoding.

Blocking problems:                                           
I'd like to ask for a cleanup of physical structure of scenarios.
As in reboot is not basic, neither is audio/opengl stuff.
The configs cleanup.    
                                                             
./setup test fails, no dependency on yaml declared.          
                                                             
                                                             
Non-blocking issues (Don't have to be resolved at this time):
                                                                                        
It'd be nicer if yaml files had a '.yaml' suffix.            
                                                             
In the scenarios I'd not match against fancy unicode (checkmark). Does remote even print that glyph?
                                                             
In the steps:                                                
- Send is a misnomer. Send signal? Send file? It's for typing keys, so TBH, it should just be PushKeys, Type, or soemthing that's close to reality.
For the keys themselves, I think having to use keys.KEY_SPACE for ' ' is bad. The spaces from the string should get auto translated into that. Same goes for enter (\n or \r).
                                                             
I still think that no audio or rendering should happen. It's just against test design principles. If checkbox is responsible for forwarding some environment info that's needed for audio tests then we should test for those variables, not variables _and_ actual hardware. This frees up dependencies, time, etc.
                                                             
I still think that assertions are not actions. Assertions should be checked against completed scenario. Like assertion on output or a report. The action is expect, where indeed there is a sequence of things that happen.
                                                             
The setup commands run in the provider are worse than what we originally had. Less readable, with more escaping and bashisms.
                                                             
Encoding re patterns to bytes and running searches on bytes is backwards. The output that's being searched should be decoded using stdout's encoding. Instead of hardcoding the pattern encoding.

review: Needs Fixing

Revision history for this message

Sylvain Pineau (sylvain-pineau) wrote on 2021-03-30:

#

Fixed blocking issues, scn structure is not perfect though.

review: Needs Resubmitting

Revision history for this message

Maciej Kisielewski (kissiel) wrote on 2021-03-31:

#

We'll improve as we go.

review: Approve

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Checkbox

Merge ~sylvain-pineau/checkbox/+git/metabox:configs into ~checkbox-dev/checkbox/+git/metabox:main

Commit message

Description of the change

Preview Diff

Subscribers