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

Get rid of remote_queue from browsers #49838

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gsnedders
Copy link
Member

This doesn't belong here, especially given it was only ever used to
send "log" commands (which there's no need for: we're within the same
process, we can just invoke the logger directly).

While we're at it, migrate LogcatRunner over to using the same
OutputHandler as used elsewhere (e.g., for WebDriver), thus hopefully
avoiding further duplication.

This doesn't belong here, especially given it was only ever used to
send "log" commands (which there's no need for: we're within the same
process, we can just invoke the logger directly).

While we're at it, migrate LogcatRunner over to using the same
OutputHandler as used elsewhere (e.g., for WebDriver), thus hopefully
avoiding further duplication.
@wpt-pr-bot wpt-pr-bot added infra wptrunner The automated test runner, commonly called through ./wpt run labels Dec 24, 2024
@gsnedders
Copy link
Member Author

Note that if testrunner.py was checking untyped defs we'd be getting, because Browser doesn't expect the remote_queue argument:

tools/wptrunner/wptrunner/testrunner.py:767: error: Unexpected keyword argument "remote_queue" for "Browser"  [call-arg]
tools/wptrunner/wptrunner/browsers/base.py:106: note: "Browser" defined here

@WeizhongX
Copy link
Contributor

The reason it was implemented this way is because we want to associate the logcat logs with test run at the time. In Chromium we have the capability to browse logs/traces associated with a test through html file, thus make it easy for triage.

Android system will usually produce a lot of logcat logs. Log all of them will flood the terminal. And we have wptrunner/wptrunner/formatters/chromium.py to handle that.

@gsnedders
Copy link
Member Author

The reason it was implemented this way is because we want to associate the logcat logs with test run at the time. In Chromium we have the capability to browse logs/traces associated with a test through html file, thus make it easy for triage.

Android system will usually produce a lot of logcat logs. Log all of them will flood the terminal. And we have wptrunner/wptrunner/formatters/chromium.py to handle that.

OK, and this should go from:

data = {
"action": "process_output",
"process": "LOGCAT",
"command": "logcat",
"data": line
}

To:

self.logger.process_output(self.pid,
line.decode("utf8", "replace"),
command=" ".join(self.command) if self.command else "")

This does slightly changes what gets logged, but that shouldn't change things about how the association happens, changing the command from the constant logcat to the actual invoked command, and changes the process name from LOGCAT to the pid. But this just moves us to matching what's recommended in our mozlog fork's docstring:

"""Log output from a managed process.
:param process: A unique identifier for the process producing the output
(typically the pid)
:param data: The output to log
:param command: Optional string representing the full command line used to start
the process.
:param test: Optional ID of the test which the process was running.
:param subsuite: Optional name of the subsuite which the process was running.
"""

It shouldn't however have any impact on when things get logged? In either case, it should be when the mozprocess output handler gets called, which will then trigger a log event to get sent.

Even if you absolutely want to maintain the same output, we should move from self.remote_queue.send_message(("log", {"action": "process_output", …})) to self.logger.process_output(…), because we gain nothing by passing the remote_queue to here. There is literally no difference in behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra wptrunner The automated test runner, commonly called through ./wpt run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants