-
Notifications
You must be signed in to change notification settings - Fork 1
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
Implement high-level integration tests for the reference application #118
Conversation
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.
This is a bug fix. self.frame_count
accessed in match()
could contain a frame number out of sync with the grabbed screenshot. This fix makes sure that the frame count checked in match()
is the actual frame count when grab_screenshot()
was called.
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.
Would this be better in another PR?
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.
Moved to #119.
Tests are failing by the way @hbatagelo , but I am reviewing! |
Oh wait. Is that because canonical/mir-test-tools#76 isn't in yet? |
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 have some initial questions and minor feedback!
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.
Would this be better in another PR?
mir-ci/mir_ci/fixtures/servers.py
Outdated
@@ -22,7 +22,8 @@ class ServerCap(Flag): | |||
DISPLAY_CONFIG = auto() | |||
SCREENCOPY = auto() | |||
INPUT_METHOD = auto() | |||
ALL = FLOATING_WINDOWS | DRAG_AND_DROP | DISPLAY_CONFIG | SCREENCOPY | INPUT_METHOD | |||
MIR_FLUTTER_APP = auto() | |||
ALL = FLOATING_WINDOWS | DRAG_AND_DROP | DISPLAY_CONFIG | SCREENCOPY | INPUT_METHOD | MIR_FLUTTER_APP |
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.
It's kinda weird that the rest of these are server capabilities, and then there's MIR_FLUTTER_APP
. Could this instead be a MIR_SHELL_PROTO
capability (denoting the support for the mir_shell
protocol), and then have the test case run the flutter app against the server selected?
That would fit in better with how the rest of these tests work, and also allow us to test different servers that support mir_shell
? (ie: maybe we want to test both miriway and miral-shell?)
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.
Yes, mir_demo_server.mir_flutter_app
is just the demo server running an app. It doesn't seem to fit here, but do I have another option? Using MIR_SHELL_PROTO
would work great if, instead of bundling mir_flutter_app
into mir-test-tools
, mir_flutter_app
had its own snap. This was actually what I did in my first commit, running a mir_flutter_app snap in the servers that support mir_shell, like miriway and miral-shell (IIRC, I used MIR_SHELL to tag the compatible servers, but then changed to MIR_FLUTTER_APP).
In the current state, MIR_SHELL_PROTO
can only be used with mir_demo_server.mir_flutter_app
because test_mir_flutter_app.py
won't run any application as a subprocess. If we tag miriway or confined_server with MIR_SHELL_PROTO
, the test will just time out because there will be no application to test.
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.
@hbatagelo why do we have the reference app "bundled" with a server? Why not just run a server like we usually do, and run the app itself separately, like we do in other tests?
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.
@hbatagelo why do we have the reference app "bundled" with a server? Why not just run a server like we usually do, and run the app itself separately, like we do in other tests?
Oh, I thought MIRENG-479 was about that. I used the other mir-test-tools apps (sdl22-test, qt-test, etc) as a reference, as they are all invoked by mir_demo_server. I'll change the mir-flutter-app snap "app" so it calls mir_flutter_app directly.
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 would also rather this follow the OSK test MO, where the robot file is a static one stored in the suites and assets are collected into a single folder.
A little copy-pasta now, but will be easier to maintain going forward.
Another thing I'd consider is applying the same "proportional geometry" approach I used in OSKMap.py
to reduce the amount of templates that need updating for different variants etc. Maybe even a MirApp.resource
makes sense here?
|
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.
One small nit, but this looks good to me otherwsie
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.
This looks good to me, now.
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.
With my final nit out of the way, I would be happy to see this merge (maybe if we can make CI a little more green if possible)
Need to resolve the servers.py conflict. Rebasing on |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #118 +/- ##
==========================================
- Coverage 68.92% 68.68% -0.24%
==========================================
Files 16 16
Lines 872 875 +3
Branches 126 126
==========================================
Hits 601 601
- Misses 247 249 +2
- Partials 24 25 +1 ☔ View full report in Codecov by Sentry. |
207c8cb
to
ef74b90
Compare
The FlutterApp test fails on mir-test-tools, you can find the failure log in the test-results artifact: https://github.com/canonical/mir-ci/actions/runs/9272416938?pr=118 It's there in |
a6f2868
to
7b3444b
Compare
Adds integration tests for the reference application.
This requires mir-flutter-app to be bundled into the mir-test-tools snap (related PR).