An Sung (Gerrit)

unread,

Apr 23, 2024, 7:53:48 PMApr 23

to Jonathan Lee, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Jonathan Lee

New activity on the change

Open in Gerrit

Related details

Attention is currently required from:

Jonathan Lee

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jonathan Lee (Gerrit)

unread,

Apr 23, 2024, 8:45:42 PMApr 23

to An Sung, Weizhong Xia, chromium...@chromium.org, blink-...@chromium.org

Attention needed from An Sung and Weizhong Xia

Jonathan Lee added 6 comments

File third_party/blink/tools/blinkpy/web_tests/vts_notifier.py

Line 47, Patchset 1 (Latest): self.options = self.parse_args(argv)

Jonathan Lee . unresolved

nit: Does this need to be an attribute, or could it just remain a local variable?

Line 48, Patchset 1 (Latest):

        if self.options.verbose:
            configure_logging(logging_level=logging.DEBUG, include_time=True)

Jonathan Lee . unresolved

We should always `configure_logging()` instead of skipping it for `verbose=False`. Use the `verbose` flag to just [pass a different level](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/tools/blinkpy/w3c/test_importer.py;l=106;drc=a3a5890e78d541dca55f46921866f74b89fab5d2;bpv=0;bpt=0) instead:


```suggestion
        level = logging.DEBUG if self.options.verbose else logging.INFO
        configure_logging(logging_level=level, include_time=True)
```

Line 50, Patchset 1 (Latest): self._dry_run = self.options.dry_run

Jonathan Lee . unresolved

I think a test that calls `process_draft_bug()` without calling `run()` first will crash. Generally, [`__init__` should define all attributes](https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/attribute-defined-outside-init.html), even if it's just a default like `self._dry_run = False` that `run()` will overwrite.

Alternatively, you can construct the desired object to begin with using a [`from_args()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/tools/blinkpy/wpt_tests/wpt_adapter.py;l=156-176;drc=258547b3a37233e95e1b1ca65e4e24f717f64875;bpv=0;bpt=0) factory.

Line 68, Patchset 1 (Latest): help='log extra details that may be helpful when debugging')

Jonathan Lee . unresolved

nit: Capitalize and add punctuation to be consistent with formatting for the `--dry-run` help.

Line 81, Patchset 1 (Latest):

            if not existing_bugs:
                if self._dry_run:

Jonathan Lee . unresolved

nit: Maybe flatten the conditional structure here instead of nesting:

```
if existing_bugs:
    _log.info('Bug already created: {...}')
elif self._dry_run:
    _log.info('Would have created the following bug: {bug}')
else:
    bug = ...
```

Line 84, Patchset 1 (Latest): f'{str(bug)}')

Jonathan Lee . unresolved

nit: No need for `str`, it should be implied.

Open in Gerrit

Related details

Attention is currently required from:

An Sung
Weizhong Xia

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

An Sung (Gerrit)

unread,

Apr 23, 2024, 9:10:07 PMApr 23

to Weizhong Xia, Jonathan Lee, chromium...@chromium.org, blink-...@chromium.org

Attention needed from Jonathan Lee

An Sung added 7 comments

Patchset-level comments

File-level comment, Patchset 1:

An Sung . resolved

Thanks for the great suggestions. A lot of patterns in this CL were made in reference to tools/blinkpy/w3c/wpt_uploader.py. Do you think it is worthwhile to apply the same improvements on it in future CL?

File third_party/blink/tools/blinkpy/web_tests/vts_notifier.py

Line 47, Patchset 1: self.options = self.parse_args(argv)

Jonathan Lee . resolved

nit: Does this need to be an attribute, or could it just remain a local variable?

An Sung

Done

Line 48, Patchset 1:

        if self.options.verbose:
            configure_logging(logging_level=logging.DEBUG, include_time=True)

Jonathan Lee . resolved

We should always `configure_logging()` instead of skipping it for `verbose=False`. Use the `verbose` flag to just [pass a different level](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/tools/blinkpy/w3c/test_importer.py;l=106;drc=a3a5890e78d541dca55f46921866f74b89fab5d2;bpv=0;bpt=0) instead:
```suggestion
        level = logging.DEBUG if self.options.verbose else logging.INFO
        configure_logging(logging_level=level, include_time=True)
```

An Sung

Done

Line 50, Patchset 1: self._dry_run = self.options.dry_run

Jonathan Lee . resolved

I think a test that calls `process_draft_bug()` without calling `run()` first will crash. Generally, [`__init__` should define all attributes](https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/attribute-defined-outside-init.html), even if it's just a default like `self._dry_run = False` that `run()` will overwrite.
Alternatively, you can construct the desired object to begin with using a [`from_args()`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/tools/blinkpy/wpt_tests/wpt_adapter.py;l=156-176;drc=258547b3a37233e95e1b1ca65e4e24f717f64875;bpv=0;bpt=0) factory.

An Sung

Done

Line 68, Patchset 1: help='log extra details that may be helpful when debugging')

Jonathan Lee . resolved

nit: Capitalize and add punctuation to be consistent with formatting for the `--dry-run` help.

An Sung

Done

Line 81, Patchset 1:

            if not existing_bugs:
                if self._dry_run:

Jonathan Lee . resolved

nit: Maybe flatten the conditional structure here instead of nesting:

```
if existing_bugs:
    _log.info('Bug already created: {...}')
elif self._dry_run:
    _log.info('Would have created the following bug: {bug}')
else:
    bug = ...
```

An Sung

Done

Line 84, Patchset 1: f'{str(bug)}')

Jonathan Lee . resolved

nit: No need for `str`, it should be implied.

An Sung

Done

Open in Gerrit

Related details

Attention is currently required from:

Jonathan Lee

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Jonathan Lee (Gerrit)

unread,

Apr 23, 2024, 10:00:13 PMApr 23

to An Sung, Weizhong Xia, chromium...@chromium.org, blink-...@chromium.org

Attention needed from An Sung

Jonathan Lee voted and added 1 comment

Votes added by Jonathan Lee

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 1:

An Sung . resolved

Thanks for the great suggestions. A lot of patterns in this CL were made in reference to tools/blinkpy/w3c/wpt_uploader.py. Do you think it is worthwhile to apply the same improvements on it in future CL?

Jonathan Lee

Do you think it is worthwhile to apply the same improvements on it in future CL?

My suggestions are very minor, so it's fine to leave the old code as-is.

Open in Gerrit

Related details

Attention is currently required from:

An Sung

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Weizhong Xia (Gerrit)

unread,

Apr 24, 2024, 11:48:23 AMApr 24

to An Sung, Jonathan Lee, chromium...@chromium.org, blink-...@chromium.org

Attention needed from An Sung

Weizhong Xia voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

An Sung

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

An Sung (Gerrit)

unread,

Apr 24, 2024, 1:13:06 PMApr 24

to Weizhong Xia, Jonathan Lee, chromium...@chromium.org, blink-...@chromium.org

An Sung voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Apr 24, 2024, 1:42:10 PMApr 24

to An Sung, Weizhong Xia, Jonathan Lee, chromium...@chromium.org, blink-...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[Blink] Add dryrun and verbose arg to vts notifier

Bug: 335457566

Change-Id: Ia11efb0d7df8a60725bb8020aa0ab56358e28b27

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5480077

Reviewed-by: Jonathan Lee <jonath...@google.com>

Reviewed-by: Weizhong Xia <weiz...@google.com>

Commit-Queue: An Sung <ans...@google.com>

Cr-Commit-Position: refs/heads/main@{#1291972}

Files:

M third_party/blink/tools/blinkpy/web_tests/vts_notifier.py
M third_party/blink/tools/blinkpy/web_tests/vts_notifier_unittest.py

Change size: S

Delta: 2 files changed, 28 insertions(+), 6 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Weizhong Xia, +1 by Jonathan Lee

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[Blink] Add dryrun and verbose arg to vts notifier [chromium/src : main]

An Sung (Gerrit)

New activity on the change

Related details

Jonathan Lee (Gerrit)

Jonathan Lee added 6 comments

Related details

An Sung (Gerrit)

An Sung added 7 comments

Related details

Jonathan Lee (Gerrit)

Jonathan Lee voted and added 1 comment

Votes added by Jonathan Lee

1 comment

Related details

Weizhong Xia (Gerrit)

Weizhong Xia voted Code-Review+1

Related details

An Sung (Gerrit)

An Sung voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information