-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
terminal: notify terminal emulator about test session progress #13728
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
Conversation
This replaces PR #13395. While searching the web for OSC 9;4, I found some reports that this sequence conflicts with OSC 9 usage by the iTerm terminal emulator, which uses it for generating desktop notifications. So this might end up generating a bunch of notifications on such terminals... I will need to test if that is indeed the case. Though I don't have easy access to macOS so if someone's able to test it that would be appreciated :) For reference I tested it on:
|
For what it's worth, I can recommend Scaleway for this. They charge 0.11€/h for a Mac Mini you can access remotely, so you pay 2.64€ for the minimum of 24h to try something out. Could easily get it back after via our OpenCollective too. Only caveat is that you need to remember to disable it after 24h, because you can't do that in advance sadly. |
@bluetech could you include links to relevant docs/standards for future reference? |
Is https://conemu.github.io/en/AnsiEscapeCodes.html#ConEmu_specific_OSC the canonical standard? |
@The-Compiler thanks for the suggestion, in the end I was able to get access to an old apple laptop. @webknjaz Yes, I think ConEmu invented these sequences. There is a link in the code, but I will also link it in the changelog. |
The `write` method does some processing on the input, namely markup and adding to the current line. `write_raw` skips these parts. It is useful for emitting invisible escape sequences.
Return how many items have been reported in the progress so far. This cleans up the code a bit, but also allows grabbing this info without accessing the internals. We can consider making this property public separately.
c99e5ed
to
f1b1c0a
Compare
|
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.
Thanks! Tested it and the feature works well on Windows.
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 added a couple of thoughts - mostly worried about the things in the changelog, the rest is "take it or leave it" material in my book 😉
|
||
# OSC 9;4 sequence: ESC ] 9 ; 4 ; state ; progress ST | ||
# ST can be ESC \ or BEL. ESC \ seems better supported. | ||
match state: |
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.
Since you say Changed the code to use match
, reads more nicely I'll take the freedom to nitpick on this:
Wouldn't it be simpler to say something like (untested):
state_ids = {
"remove": 0,
"normal": 1,
"error": 2,
"indeterminate": 3,
"paused": 4,
}
state_id = state_ids[state]
if progress is None:
assert state != "normal"
progress = ""
else:
assert state not in ["remove", "indeterminate"], state
sequence = f"\x1b]9;4;{state_id};{progress}\x1b\\"
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 find this harder to read, so somewhat prefer the match
version. Just straight dumb code :)
changelog/13072.feature.rst
Outdated
@@ -0,0 +1,6 @@ | |||
Added support for displaying test session progress in the terminal tab using the `OSC 9;4; <https://conemu.github.io/en/AnsiEscapeCodes.html#ConEmu_specific_OSC>`_ ANSI sequence. | |||
When pytest runs in a supported terminal emulator like ConEmu, Gnome Terminal, Ptyxis, Windows Terminal or Ghostty, |
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.
Kitty as well maybe?
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.
Will add!
changelog/13072.feature.rst
Outdated
you'll see the progress in the terminal tab or window, | ||
allowing you to monitor pytest's progress at a glance. | ||
|
||
This feature is automatically enabled. |
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.
Should we really do that? If yes, should we maybe mention -p no:terminalprogress
as an escape hatch to disable it?
With e.g. stable Linux distributions, it's entirely possible people will continue using affected versions (that show notifications) for possibly years. Has someone checked how that situation looks like on e.g. Ubuntu 22.04 (supported until 2027) or 24.04 (until 2029)?
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.
Yea, I'll mention it.
Regarding possible breakage, I'm definitely concerned about it. But I think we should just try it and see. I understand that smells like the derided "move fast and break stuff" approach, but I am not able to do full testing on other distros myself unfortunately. I am a bit optimistic because:
- Not a lot of terminal emulators implemented the desktop notification sequence
- None of the default terminal emulators (which probably most people use) implemented it AFAIK
- systemd already uses it (though admittedly that's linux only and much less like to be running on older distros than pytest).
It's possible we'll need to change it to off-by-default if there are complaints.
testing/test_terminal.py
Outdated
plugin._emit_progress("indeterminate") | ||
assert "\x1b]9;4;3;\x1b\\" in mock_file.getvalue() | ||
mock_file.truncate(0) | ||
mock_file.seek(0) |
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.
Parametrize the test case maybe?
f1b1c0a
to
a090c83
Compare
@The-Compiler thanks for the review. I updated the 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.
Thanks, let's get this in and see what people say!
Use OSC 9;4 ANSI sequences terminal progress to notify the terminal emulator about progress, so it can display it to the user (e.g. on the terminal tab). Fix pytest-dev#13072. Co-Authored-By: Anna Tasiopoulou <[email protected]>
a090c83
to
99cd7a2
Compare
Use OSC 9;4 ANSI sequences terminal progress to notify the terminal emulator about progress, so it can display it to the user (e.g. on the terminal tab).
Fix #13072.