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

Fixes exception during HandshakeException handler #27791

Merged

Conversation

@arenevier
Copy link
Contributor

@arenevier arenevier commented Feb 25, 2021

Make sure we pass only one argument to logger.info

Fixes #27785

Make sure we pass only one argument to logger.info

Fixes #27785
@arenevier
Copy link
Contributor Author

@arenevier arenevier commented Feb 25, 2021

Something we could do instead (or additionally), to prevent that from happening again is to modify NoOpLogger methods signatures to accept multiple arguments (ie def info(self, *msg))

@arenevier
Copy link
Contributor Author

@arenevier arenevier commented Feb 25, 2021

@ricea
ricea approved these changes Feb 26, 2021
Copy link
Contributor

@ricea ricea left a comment

lgtm

@stephenmcgruer
Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Feb 26, 2021

Something we could do instead (or additionally), to prevent that from happening again is to modify NoOpLogger methods signatures to accept multiple arguments (ie def info(self, *msg))

I think that yes, NoOpLogger should have the same signature as the logger we are trying to use (which I believe is just https://docs.python.org/3/library/logging.html ?). So based on e.g. https://docs.python.org/3/library/logging.html#logging.Logger.info, it should be def info(msg, *args, **kwargs), but I'd be interested if @jgraham agrees or knows something otherwise :D

@jgraham
Copy link
Contributor

@jgraham jgraham commented Feb 26, 2021

The logger can be a mozlog logger which doesn't support he string interpolation feature so unless we change mozlog, the current signature is correct.

@stephenmcgruer
Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Feb 26, 2021

The logger can be a mozlog logger which doesn't support he string interpolation feature so unless we change mozlog, the current signature is correct.

Ah, that is great to know! So our current log statements should never use the _log.info("string %s", argument) form, then? In which case this PR is 100% correct :)

@stephenmcgruer stephenmcgruer merged commit 4f24e1d into web-platform-tests:master Feb 26, 2021
43 of 45 checks passed
43 of 45 checks passed
@github-actions
update-pr-preview
Details
@github-actions
update-pr-preview
Details
@github-actions
update-pr-preview
Details
@github-actions
build-and-publish
Details
@github-actions
build-and-tag
Details
@azure-pipelines
Azure Pipelines Build #20210225.89 failed
Details
@azure-pipelines
Azure Pipelines (infrastructure/ tests: macOS) infrastructure/ tests: macOS failed
Details
@azure-pipelines
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
@azure-pipelines
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
@azure-pipelines
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
@azure-pipelines
Azure Pipelines (tools/ unittests: Windows + Python 3.6) tools/ unittests: Windows + Python 3.6 succeeded
Details
@azure-pipelines
Azure Pipelines (tools/ unittests: Windows + Python 3.8) tools/ unittests: Windows + Python 3.8 succeeded
Details
@azure-pipelines
Azure Pipelines (tools/ unittests: macOS + Python 3.6) tools/ unittests: macOS + Python 3.6 succeeded
Details
@azure-pipelines
Azure Pipelines (tools/ unittests: macOS + Python 3.8) tools/ unittests: macOS + Python 3.8 succeeded
Details
@azure-pipelines
Azure Pipelines (tools/wpt/ tests: Windows + Python 3.6) tools/wpt/ tests: Windows + Python 3.6 succeeded
Details
@azure-pipelines
Azure Pipelines (tools/wpt/ tests: Windows + Python 3.8) tools/wpt/ tests: Windows + Python 3.8 succeeded
Details
@azure-pipelines
Azure Pipelines (tools/wpt/ tests: macOS + Python 3.6) tools/wpt/ tests: macOS + Python 3.6 succeeded
Details
@azure-pipelines
Azure Pipelines (tools/wpt/ tests: macOS + Python 3.8) tools/wpt/ tests: macOS + Python 3.8 succeeded
Details
@azure-pipelines
Azure Pipelines (tools/wptrunner/ unittests: Windows + Python 3.6) tools/wptrunner/ unittests: Windows + Python 3.6 succeeded
Details
@azure-pipelines
Azure Pipelines (tools/wptrunner/ unittests: Windows + Python 3.8) tools/wptrunner/ unittests: Windows + Python 3.8 succeeded
Details
@azure-pipelines
Azure Pipelines (tools/wptrunner/ unittests: macOS + Python 3.6) tools/wptrunner/ unittests: macOS + Python 3.6 succeeded
Details
@azure-pipelines
Azure Pipelines (tools/wptrunner/ unittests: macOS + Python 3.8) tools/wptrunner/ unittests: macOS + Python 3.8 succeeded
Details
@azure-pipelines
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
@azure-pipelines
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
@community-tc-integration
download-firefox-nightly Community-TC (pull_request)
Details
@community-tc-integration
infrastructure/ tests (Python 3) Community-TC (pull_request)
Details
@community-tc-integration
lint Community-TC (pull_request)
Details
@community-tc-integration
resources/ tests (Python 3.6) Community-TC (pull_request)
Details
@community-tc-integration
resources/ tests (Python 3.8) Community-TC (pull_request)
Details
@community-tc-integration
sink-task Community-TC (pull_request)
Details
@community-tc-integration
tools/ integration tests (Python 3.6) Community-TC (pull_request)
Details
@community-tc-integration
tools/ integration tests (Python 3.8) Community-TC (pull_request)
Details
@community-tc-integration
tools/ unittests (Python 3.6) Community-TC (pull_request)
Details
@community-tc-integration
tools/ unittests (Python 3.8) Community-TC (pull_request)
Details
@community-tc-integration
update-built Community-TC (pull_request)
Details
@community-tc-integration
wpt-chrome-dev-results Community-TC (pull_request)
Details
@community-tc-integration
wpt-chrome-dev-results-without-changes Community-TC (pull_request)
Details
@community-tc-integration
wpt-chrome-dev-stability Community-TC (pull_request)
Details
@community-tc-integration
wpt-decision-task Community-TC (pull_request)
Details
@community-tc-integration
wpt-firefox-nightly-results Community-TC (pull_request)
Details
@community-tc-integration
wpt-firefox-nightly-results-without-changes Community-TC (pull_request)
Details
@community-tc-integration
wpt-firefox-nightly-stability Community-TC (pull_request)
Details
@wpt-fyi
wpt.fyi - chrome[experimental] Chrome results
Details
@wpt-fyi
wpt.fyi - firefox[experimental] Firefox results
Details
@wpt-fyi
wpt.fyi - safari[experimental] Safari results
Details
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.

5 participants