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

Do not print logs twice when running wpt lint #10148

Merged
merged 1 commit into from Apr 17, 2018

Conversation

Projects
None yet
6 participants
@bmac
Copy link
Contributor

bmac commented Mar 22, 2018

Running wpt lint appears to print a duplicate line to the console for each line the logger module prints.

Before

$ ./wpt lint cors/allow-headers.htm 
cors/allow-headers.htm:87: Whitespace at EOL (TRAILING WHITESPACE)
ERROR:lint:cors/allow-headers.htm:87: Whitespace at EOL (TRAILING WHITESPACE)

INFO:lint:
There was 1 error (TRAILING WHITESPACE: 1)
INFO:lint:There was 1 error (TRAILING WHITESPACE: 1)
You must fix all errors; for details on how to fix them, see
INFO:lint:You must fix all errors; for details on how to fix them, see
http://web-platform-tests.org/writing-tests/lint-tool.html
INFO:lint:http://web-platform-tests.org/writing-tests/lint-tool.html

INFO:lint:
However, instead of fixing a particular error, it's sometimes
INFO:lint:However, instead of fixing a particular error, it's sometimes
OK to add a line to the lint.whitelist file in the root of the
INFO:lint:OK to add a line to the lint.whitelist file in the root of the
web-platform-tests directory to make the lint tool ignore it.
INFO:lint:web-platform-tests directory to make the lint tool ignore it.

INFO:lint:
For example, to make the lint tool ignore all 'TRAILING WHITESPACE'
INFO:lint:For example, to make the lint tool ignore all 'TRAILING WHITESPACE'
errors in the cors/allow-headers.htm file,
INFO:lint:errors in the cors/allow-headers.htm file,
you could add the following line to the lint.whitelist file.
INFO:lint:you could add the following line to the lint.whitelist file.

INFO:lint:
TRAILING WHITESPACE: cors/allow-headers.htm
INFO:lint:TRAILING WHITESPACE: cors/allow-headers.htm

After

$ ./wpt lint cors/allow-headers.htm 
cors/allow-headers.htm:87: Whitespace at EOL (TRAILING WHITESPACE)

There was 1 error (TRAILING WHITESPACE: 1)
You must fix all errors; for details on how to fix them, see
http://web-platform-tests.org/writing-tests/lint-tool.html

However, instead of fixing a particular error, it's sometimes
OK to add a line to the lint.whitelist file in the root of the
web-platform-tests directory to make the lint tool ignore it.

For example, to make the lint tool ignore all 'TRAILING WHITESPACE'
errors in the cors/allow-headers.htm file,
you could add the following line to the lint.whitelist file.

TRAILING WHITESPACE: cors/allow-headers.htm

This change is Reviewable

@wpt-pr-bot wpt-pr-bot added the infra label Mar 22, 2018

@wpt-pr-bot wpt-pr-bot requested review from gsnedders and jgraham Mar 22, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 22, 2018

Build PASSED

Started: 2018-03-23 18:25:09
Finished: 2018-03-23 18:42:44

View more information about this build on:

@Cactusmachete

This comment has been minimized.

Copy link
Contributor

Cactusmachete commented Mar 23, 2018

I tried to do something similar in #10147 but I think this fails because caplog isn't able to get the lint logger's logs if we set propagate to False.
I think it should work if we set the lint logger's propagate to True in the beginning of test_lint.py, though.

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Mar 23, 2018

Maybe the right solution is to not add a stream handler to this logger if there's already one attached to the root. I suspect this regressed when we switched to wpt lint. So it might be enough to just stop attaching the logger at all and assume that the root logger is always set up, but more conservatively we could check if the root logger has a handler and if not attach it there.

@bmac bmac force-pushed the bmac:noisy-linting branch from 9919d25 to ab7222f Mar 23, 2018

@bmac

This comment has been minimized.

Copy link
Contributor Author

bmac commented Mar 23, 2018

Thanks @Cactusmachete and @jgraham. I've updated the pr to only add a logger handler when the parent logger is missing a handler.

@foolip

foolip approved these changes Mar 30, 2018

Copy link
Contributor

foolip left a comment

Sweet, I've been annoyed by this many times but could never figure out how to fix it in the 5 minutes I spent each time.

Please still wait for @jgraham to review also.

@bmac

This comment has been minimized.

Copy link
Contributor Author

bmac commented Apr 3, 2018

@jgraham I have updated this pr to avoid adding a handler if a parent handler already exists. However, after seeing #8445 I suspect we may prefer to use the lint module's handler (over the root handler) since the root handler is responsible for prefixing all of the logs with INFO:lint:. What are your thoughts?

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Apr 17, 2018

Let's merge this in lieu of spending time looking for the perfect solution; I think you might be right that carefully choosing the right logger is a better long term approach.

@jgraham jgraham merged commit 16e3f3e into web-platform-tests:master Apr 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bmac bmac deleted the bmac:noisy-linting branch Apr 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.