Skip to content
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

Test Jobs for V4L2 Compliance (New) #1653

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Conversation

tomli380576
Copy link
Contributor

@tomli380576 tomli380576 commented Dec 12, 2024

Description

This PR implements test jobs utilizing the v4l2 compliance parser. Each camera will get a job for each ioctl request, so the total number of jobs would be (n_cameras * n_ioctl_tests).

Resolved issues

Documentation

As of right now each camera will run v4l2-compliance once for every ioctl request. To test more ioctls in 1 run, pass in more ioctl names in --ioctl.

Tests

Unit tests
Test plan tested on 18, 20 VM with camera pass through, 22&24 DUTs with USB cameras.

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 3 lines in your changes missing coverage. Please review.

Project coverage is 49.10%. Comparing base (aac167f) to head (08b75fe).
Report is 52 commits behind head on main.

Files with missing lines Patch % Lines
providers/base/bin/v4l2_compliance_test.py 92.59% 1 Missing and 1 partial ⚠️
providers/resource/bin/v4l2_ioctl_resource.py 93.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1653      +/-   ##
==========================================
+ Coverage   48.96%   49.10%   +0.14%     
==========================================
  Files         371      374       +3     
  Lines       40296    40420     +124     
  Branches     6808     6834      +26     
==========================================
+ Hits        19730    19848     +118     
- Misses      19846    19848       +2     
- Partials      720      724       +4     
Flag Coverage Δ
provider-base 25.28% <92.59%> (+0.48%) ⬆️
provider-resource 35.88% <93.33%> (+0.88%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tomli380576 tomli380576 marked this pull request as ready for review January 9, 2025 06:48
@tomli380576 tomli380576 marked this pull request as draft January 17, 2025 04:32
@tomli380576 tomli380576 marked this pull request as ready for review January 17, 2025 06:16
@fernando79513 fernando79513 self-assigned this Feb 4, 2025
Copy link
Collaborator

@fernando79513 fernando79513 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.
I left a couple of small things and a comment regarding the templated jobs.
I think there is no advantage in having all of these jobs separated into templated jobs. Everything could be done in a single compliance job, but let me know your thoughts on that.

Comment on lines 34 to 45
unit: template
category_id: com.canonical.plainbox::camera
template-resource: camera/v4l2-ioctl_resource
template-unit: job
template-id: camera/v4l2-compliance_ioctl_name
_template-summary: To check if {ioctl_name} works on {name}
id: camera/v4l2-compliance_{ioctl_name}
_summary: v4l2 compliance for {ioctl_name} on {name}
plugin: shell
command:
v4l2_compliance_test.py --device /dev/{name} --ioctl {ioctl_name}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we need a templated job for this?
In my case, it creates 60+ jobs. This is not inherently bad, but I don't see the point of having them as separate jobs. The output of the test is quite straightforward, and having all of these separated tests will involve more scrolling during test review, and maybe missing some other important results.
As I said, that's a minor issue if there was an advantage of having there in separate jobs, but I think a joint report for all the compliance tests in one file will be better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used templated job for this mostly because we aren't 100% sure which ioctls are actual cert-blockers and which ones aren't. On 22.04 cameras (including USB ones that typically don't break) can fail 1 or 2 tests but the camera will appear fully functional for everyday use. Each ubuntu version also produces slightly different results for the same functioning camera. Therefore having the job templated can help us eliminate non-cert-blockers later on by filtering them out in the resource job or move them to a separate template job specifically for non-cert-blockers.

It also helps with opening/managing v4l2 bug reports since each report is related to only 1 ioctl. We don't have to read the entire jira card to see which ioctl have been fixed, which one was skipped, which one was rejected by ODM, etc.

Let me know if you think this approach makes sense. Thanks!

Copy link
Contributor Author

@tomli380576 tomli380576 Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw if the number of jobs is a concern maybe we could group some of the ioctls in the resource job? Instead of creating 1 job per ioctl, we could do 1 job per category given by v4l2-compliance. For example instead of 7 jobs we can have just 2 (Codec ioctl job and Buffer ioctl job)
image

It would look something like this

V4L2 Compliance Tests (checkbox category)
  ├─ Required ioctls (checkbox job, name is v4l2-compliance category)
  ├─ Allow for multiple opens
  ├─ Debug ioctls
  ├─ Input ioctls
  ├─ Output ioctls
  ├─ I/O Config ioctls
  ├─ Control ioctls
  ├─ Format ioctls
  ├─ Codec ioctls
  └─ Buffer ioctls

@fernando79513 fernando79513 added the waiting-for-changes The review has been completed but the PR is waiting for changes from the author label Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-changes The review has been completed but the PR is waiting for changes from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants