-
Notifications
You must be signed in to change notification settings - Fork 6
check testbot peripherals and populate contract (instead of using tags) #630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Change-type: patch Signed-off-by: Ryan Cooke <[email protected]>
|
@balena-ci rebase |
2fdd05c to
ed553d6
Compare
Change-type: patch Signed-off-by: Ryan Cooke <[email protected]>
| import { TestBotWorker } from './workers/testbot'; | ||
| import QemuWorker from './workers/qemu'; | ||
| import { Contract } from '../typings/worker'; | ||
| import * as peripherals from './peripherals/peripherals.json'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Peripherals have to be specified now? Are we making this a compulsory feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bucknalla they don't -have- to be specified. But if you want the testbot/workers contract to show that the peripheral is attached to it, you'll have to add it to that json file.
Just an idea - this isn't set in stone :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, there would have to be some quite clever scanning for attached peripherals - which maybe is a better idea now I think about it. But whatever the scan finds has to be then converted into something constrained, so the tests can determine if they should run or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rcooke-warwick I like this approach :) I just think we shouldn't assume that we need a peripherals.json and be able to assume that its a standard setup, requiring the module as an import kind of enforces that the peripherals.json must exist?
| { | ||
| "name": "USB to ethernet adapter", | ||
| "slug": "ethernet", | ||
| "cmd": "[ -d /sys/class/net/eth1 ] && echo PASS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this command is restrictive to USB ethernet adapters; also this assumes that a testbot is definitely a balenaFin which by default (no USB ethernet) should only list eth0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind this one is that if the usb to ethernet adapter is present, then it enumerates as eth1. It is assuming it's a fin actually you're right - I'm not sure what the usb to ethernet adapter shows up as if you connect it to a pi4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I like this approach but it feels a little but unconstrained. I guess there's not really a standard way to check these peripherals so its down to the user to appropriately provide the test case logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bucknalla yeah thats right. The user who wants to use a hardware peripheral (and have tests be able to determine if its present or not), is the only person who knows how to check its there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one, are you thinking to also eliminate the current implementation of how we fetch the tags using the SDK? I don't think it would be needed now.
|
@vipulgupta2048 yeah absolutely. I think this idea could do with a bit of refining but I prefer it to the tags |
|
@rcooke-warwick will you remove the tags implementation in this PR? |
|
@balena-ci rebase |
ed7fded to
0f3bfa7
Compare
|
Can one of the admins verify this patch? |
We implemented a '/contract' endpoint on the worker container a while ago that could be used to determine what extra peripherals, such as the hdmi capture device or usb-to-ethernet adapter were connected. In the tests, contracts could then be used to determine whether or not to run the tests based on if the hardware was present.
We were using tags to determine that, but this PR changes that so that the testbot itself looks for the hw. If you want a new piece of hardware supported, you just have to add its slug (something that can be used in test contract checks), and the linux command needed to check if it's present or not.
For extra hardware connected to the DUT, like modems, this won't work. We should probably add something in the suite.js of the test suite to check if hardware is present on the DUT, and populate the DUT's hardware contract based on that.
Change-type: patch
Signed-off-by: Ryan Cooke [email protected]