Code review comment for ~sylvain-pineau/checkbox/+git/metabox:configs

Revision history for this message
Maciej Kisielewski (kissiel) wrote :

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

« Back to merge proposal