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

docs: augmenting the test plan with X11 clients #3397

Merged
merged 6 commits into from
Jun 6, 2024

Conversation

@mattkae mattkae requested a review from a team as a code owner May 23, 2024 18:04
Copy link

codecov bot commented May 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.27%. Comparing base (fdeafdf) to head (33dcdea).
Report is 444 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3397      +/-   ##
==========================================
- Coverage   77.27%   77.27%   -0.01%     
==========================================
  Files        1075     1075              
  Lines       68401    68401              
==========================================
- Hits        52857    52855       -2     
- Misses      15544    15546       +2     

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

Copy link
Contributor

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

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

I have a problem with this approach: I don't think it is scalable to add manual testing for every bug we fix.

Only if we cannot automate a test and there is a likelyhood of a future regression should we consider adding manual testing

@@ -108,6 +108,44 @@ For each empty box in the matrix above, ensure that the following applications c
gnome-terminal
```

5. Test that `X11` apps can be started and can receive input when `XWayland` is supported:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think using a specific compositor is appropriate here: e.g. the default miral-app

Comment on lines 125 to 130
6. (X11) Test that `mattermost` can be opened and resized. This was first encountered in
[#2509](https://github.com/canonical/mir/issues/2509). To test:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was a longstanding bug, not a 2.16 regression.

I don't doubt it needs testing, but this sort of arithmetic error should probably have a unit test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

canonical/mir-ci#124 I think this would be good in mir-ci since this is application-specific

Comment on lines 134 to 141
7. (X11) You will need a CLion account for this one. Test that `clion` menus can be interacted
with. This was first encountered in [#2326](https://github.com/canonical/mir/issues/2326),
[#3107](https://github.com/canonical/mir/issues/3107),
[#3024](https://github.com/canonical/mir/issues/3024).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was a longstanding bug, not a 2.16 regression. Also, this should be reproducible on any of the free, or free trial, JetBrains tools

Copy link
Contributor Author

Choose a reason for hiding this comment

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

canonical/mir-ci#125 I think that this would be good in mir-ci as wel since its app specific

Comment on lines 225 to 232
2. Set orientation of your outputs in `$FILENAME` [normal, left, right, inverted]
3. Confirm that the output has the correctly rotated content
Copy link
Contributor

Choose a reason for hiding this comment

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

This is easier to test with

miral-app -demo-server --screen-rotation

When you can "Rotate screen on Ctrl-Alt-"

Comment on lines 232 to 238
2. In the configuration, have a custom attribute, e.g. `snap-name` in the following example:
Copy link
Contributor

Choose a reason for hiding this comment

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

The only compositor that supports snap-name is Frame. And that gets its display configuration from the display snap configuration option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

canonical/ubuntu-frame#184 This should be in the frame repo

gvncviewer localhost
```
3. Enter `Ctrl + Alt + Backspace`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
3. Enter `Ctrl + Alt + Backspace`
3. "Send Key" `Ctrl + Alt + Backspace`

Comment on lines 270 to 276
[#3070](https://github.com/canonical/mir/issues/3070). To test:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be tested via test automation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Saviq
Copy link
Collaborator

Saviq commented May 24, 2024

I have a problem with this approach: I don't think it is scalable to add manual testing for every bug we fix.

Only if we cannot automate a test and there is a likelyhood of a future regression should we consider adding manual testing

The idea is to record the scenarios we need to extend our testing with, and reduce the test plan as we bridge those gaps. Maybe it's a separate section, or document — things we know we can, and want to, automate? Or maybe they're just issues here, or on mir-ci?

@AlanGriffiths
Copy link
Contributor

The idea is to record the scenarios we need to extend our testing with, and reduce the test plan as we bridge those gaps. Maybe it's a separate section, or document — things we know we can, and want to, automate? Or maybe they're just issues here, or on mir-ci?

I agree we should track those we don't (yet) have tests for. I don't agree they belong in the manual Mir test plan.

@mattkae
Copy link
Contributor Author

mattkae commented May 24, 2024

@Saviq and @AlanGriffiths: I created a number of tickets on mir-ci and one on ubuntu-frame (although maybe that belongs in mir-ci too). I kept out some things that seem to have no feasible automation at the moment, but perhaps they can have something too...

Comment on lines 203 to 210
encountered in [#3238](https://github.com/canonical/mir/issues/3238). To test:
1. Sleep your session
2. Wake your session up
3. Confirm that the compositor is still running and that the contents on the
screen(s) match what should be there

This would be automated by https://github.com/canonical/mir-ci/issues/126.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be an insufficient description for the test? The bug under test specifically required that you have a clone-mode multi-monitor setup to trigger.

General sleep/wake testing would be valuable, but that's not tied to this specific bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops right! I will add that to the test description


- Confirm that the keyboard works after you have disconnected and reconnected it. This
issue was first encountere in [#3149](https://github.com/canonical/mir/issues/3149).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
issue was first encountere in [#3149](https://github.com/canonical/mir/issues/3149).
issue was first encountered in [#3149](https://github.com/canonical/mir/issues/3149).

Comment on lines 212 to 217
[#3205](https://github.com/canonical/mir/issues/3205). To test:
1. Move the cursor through many different surfaces, including Wayland and X11
windows

This would be automated by https://github.com/canonical/mir-ci/issues/127.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to link a different bug here? That bug didn't have anything to do with the cursor moving at a different speed over different surfaces? It was a Mir performance regression, where we did a big CPU copy each frame.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, the "multiple surfaces" bit was a hallucination on my part

@mattkae mattkae requested a review from RAOF May 28, 2024 11:31
Base automatically changed from docs/how_to_test_mir to main May 30, 2024 11:41
Copy link
Contributor

@RAOF RAOF left a comment

Choose a reason for hiding this comment

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

Yes, those are reasonable additions

Copy link
Contributor

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

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

Sure

@AlanGriffiths AlanGriffiths changed the title docs: augmenting the test plan to include 2.16 regressions docs: augmenting the test plan with X11 clients Jun 4, 2024
@AlanGriffiths AlanGriffiths added this pull request to the merge queue Jun 6, 2024
Merged via the queue into main with commit b25b49d Jun 6, 2024
23 of 25 checks passed
@AlanGriffiths AlanGriffiths deleted the docs/2.16_regressions branch June 6, 2024 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants