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

Always pass --enable-chrome-logs for Chromedriver #25540

Merged
merged 1 commit into from Oct 2, 2020

Conversation

@stephenmcgruer
Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Sep 15, 2020

No description provided.

@stephenmcgruer
Copy link
Contributor Author

@stephenmcgruer stephenmcgruer commented Sep 15, 2020

@Hexcles - WDYT? I pushed a full trigger run and had a poke through the logs to see if anything blew up; we definitely do get some additional logs but not a huge amount so maybe that's fine?

I think it would be nice for folks to get logs both (a) in the stability checks and (b) when running locally without them doing anything, we just need to balance it off against the risk of Chrome spamming our CI logs into submission :D.

@Hexcles
Copy link
Member

@Hexcles Hexcles commented Sep 16, 2020

This seems a bit noisy: https://community-tc.services.mozilla.com/tasks/YH7_UqpmRiSMONjq3GtPnA/runs/1/logs/https%3A%2F%2Fcommunity-tc.services.mozilla.com%2Fapi%2Fqueue%2Fv1%2Ftask%2FYH7_UqpmRiSMONjq3GtPnA%2Fruns%2F1%2Fartifacts%2Fpublic%2Flogs%2Flive.log

The verbosity might be OK (I'm a bit on the fence here), but I'd like to at least tell Chrome logs apart from runner logs more easily, e.g. adding a prefix to logs (I know right now we can filter by "pid:" but that seems a bit brittle and not obvious). Perhaps we can put Chrome logs into a log file as an artifact instead?

@stephenmcgruer
Copy link
Contributor Author

@stephenmcgruer stephenmcgruer commented Sep 16, 2020

The verbosity might be OK (I'm a bit on the fence here), but I'd like to at least tell Chrome logs apart from runner logs more easily, e.g. adding a prefix to logs (I know right now we can filter by "pid:" but that seems a bit brittle and not obvious). Perhaps we can put Chrome logs into a log file as an artifact instead?

I'm not sure if we want to do that, as it then becomes hard to find where one test ends and another begins (unless we also dump information on that to the log).

I think there's two main angles we want to cover here:

  1. Developers tell us that looking at WPT CI logs are useless (especially for crashes) as they don't include anything from the browser.
  2. Developers tell us that they want access to browser logs locally when running WPT.

If we think always-on is too spammy, then I would propose we go forward by turning this flag on for stability checks specifically (as they tend to be the most likely CI that a dev is looking at), and otherwise instruct developers to pass --webdriver-args=--enable-chrome-logs when running locally. (That may be worth wrapping in an easier to understand flag).

Copy link
Member

@Hexcles Hexcles left a comment

Thanks, Stephen! I'm convinced that we want browser logs included by default; in other words, I'm not worried about the verbosity any more.

Going back to my other point about interleaving two kinds of logs, I agree that splitting Chrome logs into a seperate file defeats the purpose, so it seems the best way is to prefix the runner logs and/or the browser logs, but I'm fine with leaving that as a future improvement.

@stephenmcgruer
Copy link
Contributor Author

@stephenmcgruer stephenmcgruer commented Sep 16, 2020

it seems the best way is to prefix the runner logs and/or the browser logs, but I'm fine with leaving that as a future improvement.

That would be done in on_output in WebDriverServer (tools/wptrunner/wptrunner/webdriver_server.py). It uses mozlog to log the output:

    def on_output(self, line):
        self.logger.process_output(self.pid,
                                   line.decode("utf8", "replace"),
                                   command=" ".join(self._cmd))

We'd probably have to change the data itself, by prefixing something to the line.decode(...) part, since mozlog doesn't give us more more flexibility there afaik.

pull bot pushed a commit to Qwerty0x64/chromium that referenced this pull request Sep 17, 2020
Passing an async function to async_test will soon be disallowed in WPT,
as any asserts thrown will be turned into unhandled promise rejections
and the test will timeout. If a test needs async functions it should use
promise_test instead - however in this case the tests don't actually
need async functions (they don't use await or rely on promises).

Bug: web-platform-tests/wpt#25540
Change-Id: I9be1f0b89350b611ed7db260e9ec5b79def03305
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2414758
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807916}
@stephenmcgruer stephenmcgruer reopened this Oct 2, 2020
@stephenmcgruer stephenmcgruer merged commit 2939f4a into master Oct 2, 2020
73 checks passed
73 checks passed
build-and-publish
Details
build-and-publish
Details
build-and-publish
Details
build-and-publish
Details
build-and-tag
Details
build-and-tag
Details
build-and-tag
Details
build-and-tag
Details
detect-deployment
Details
Azure Pipelines Build #20201002.36 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (infrastructure/ tests: macOS) infrastructure/ tests: macOS succeeded
Details
Azure Pipelines (tools/ unittests: Windows + Python 3.6) tools/ unittests: Windows + Python 3.6 succeeded
Details
Azure Pipelines (tools/ unittests: Windows + Python 3.8) tools/ unittests: Windows + Python 3.8 succeeded
Details
Azure Pipelines (tools/ unittests: Windows) tools/ unittests: Windows succeeded
Details
Azure Pipelines (tools/ unittests: macOS + Python 3.6) tools/ unittests: macOS + Python 3.6 succeeded
Details
Azure Pipelines (tools/ unittests: macOS + Python 3.8) tools/ unittests: macOS + Python 3.8 succeeded
Details
Azure Pipelines (tools/ unittests: macOS) tools/ unittests: macOS succeeded
Details
Azure Pipelines (tools/wpt/ tests: Windows + Python 3.6) tools/wpt/ tests: Windows + Python 3.6 succeeded
Details
Azure Pipelines (tools/wpt/ tests: Windows + Python 3.8) tools/wpt/ tests: Windows + Python 3.8 succeeded
Details
Azure Pipelines (tools/wpt/ tests: Windows) tools/wpt/ tests: Windows succeeded
Details
Azure Pipelines (tools/wpt/ tests: macOS + Python 3.6) tools/wpt/ tests: macOS + Python 3.6 succeeded
Details
Azure Pipelines (tools/wpt/ tests: macOS + Python 3.8) tools/wpt/ tests: macOS + Python 3.8 succeeded
Details
Azure Pipelines (tools/wpt/ tests: macOS) tools/wpt/ tests: macOS succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: Windows + Python 3.6) tools/wptrunner/ unittests: Windows + Python 3.6 succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: Windows + Python 3.8) tools/wptrunner/ unittests: Windows + Python 3.8 succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: Windows) tools/wptrunner/ unittests: Windows succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: macOS + Python 3.6) tools/wptrunner/ unittests: macOS + Python 3.6 succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: macOS + Python 3.8) tools/wptrunner/ unittests: macOS + Python 3.8 succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests: macOS) tools/wptrunner/ unittests: macOS succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
download-firefox-nightly Community-TC (pull_request)
Details
infrastructure/ tests Community-TC (pull_request)
Details
infrastructure/ tests (Python 3) Community-TC (pull_request)
Details
lint Community-TC (pull_request)
Details
resources/ tests Community-TC (pull_request)
Details
sink-task Community-TC (pull_request)
Details
tools/ integration tests (Python 2) Community-TC (pull_request)
Details
tools/ integration tests (Python 3.6) Community-TC (pull_request)
Details
tools/ integration tests (Python 3.8) Community-TC (pull_request)
Details
tools/ unittests (Python 2) Community-TC (pull_request)
Details
tools/ unittests (Python 3.6) Community-TC (pull_request)
Details
tools/ unittests (Python 3.8) Community-TC (pull_request)
Details
wpt-chrome-dev-crashtest-1 Community-TC (push)
Details
wpt-chrome-dev-print-reftest-1 Community-TC (push)
Details
wpt-chrome-dev-reftest-1 Community-TC (push)
Details
wpt-chrome-dev-reftest-2 Community-TC (push)
Details
wpt-chrome-dev-reftest-3 Community-TC (push)
Details
wpt-chrome-dev-reftest-4 Community-TC (push)
Details
wpt-chrome-dev-reftest-5 Community-TC (push)
Details
wpt-chrome-dev-testharness-1 Community-TC (push)
Details
wpt-chrome-dev-testharness-10 Community-TC (push)
Details
wpt-chrome-dev-testharness-11 Community-TC (push)
Details
wpt-chrome-dev-testharness-12 Community-TC (push)
Details
wpt-chrome-dev-testharness-13 Community-TC (push)
Details
wpt-chrome-dev-testharness-14 Community-TC (push)
Details
wpt-chrome-dev-testharness-15 Community-TC (push)
Details
wpt-chrome-dev-testharness-16 Community-TC (push)
Details
wpt-chrome-dev-testharness-2 Community-TC (push)
Details
wpt-chrome-dev-testharness-3 Community-TC (push)
Details
wpt-chrome-dev-testharness-4 Community-TC (push)
Details
wpt-chrome-dev-testharness-5 Community-TC (push)
Details
wpt-chrome-dev-testharness-6 Community-TC (push)
Details
wpt-chrome-dev-testharness-7 Community-TC (push)
Details
wpt-chrome-dev-testharness-8 Community-TC (push)
Details
wpt-chrome-dev-testharness-9 Community-TC (push)
Details
wpt-chrome-dev-wdspec-1 Community-TC (push)
Details
wpt-chrome-dev-wdspec-2 Community-TC (push)
Details
wpt-decision-task Community-TC (pull_request)
Details
wpt.fyi - safari[experimental] Safari results
Details
@stephenmcgruer stephenmcgruer deleted the smcgruer/enable_chrome_logs branch Oct 2, 2020
ziransun added a commit to ziransun/wpt that referenced this pull request Oct 6, 2020
…#25540)

This enabled Chrome log output both for developers working locally,
as well on the CI (where we have had frequent complaints about a
lack of output, especially for crashes).

The hope is that it will not be too spammy, but we will need to see
how that goes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.