Add ci_only validator tool [chromium/src : main]
Brian Sheedy (Gerrit)
Brian Sheedy added 7 comments![]( "Open in Gerrit")
"""Discovers all trybots and target groups configuring the test suite."""As mentioned in the other review, please include docstrings with argument and return value information for any non-trivial functions/methods.
if len(parts) >= 8 and parts[4] == 'try' and parts[6] == 'targets':This seems overly complicated. The `git grep` command is already filtering to the relevant files, so we shouldn't need to do any more checking?
results.append({
'builder': builder_name,
'ci_only': is_ci_only,
})Nit: A named tuple or a dataclass is more fitting if the keys are fixed.
f" {len(trybots_and_groups)} configured trybots{revision_msg}...")Single quotes since there aren't any single quotes in the string itself.
@mock.patch('subprocess.run')Handle mocking for commonly used functions like this one in `setUp` instead of applying a decorator everywhere.
'\n'
'infra/config/generated/builders/try/android-code-coverage/targets/'
'chromium.coverage.json\n'
'infra/config/generated/builders/try/android-x86-code-coverage/'
'targets/chromium.coverage.json\n'),Use `textwrap.dedent` and a multiline string instead. Relevant here and elsewhere in this file.
@mock.patch('pathlib.Path.read_text')Shouldn't be necessary with `pyfakefs`.
Related details
- Aadarsh Battula
- Prakhar Asthana
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aadarsh Battula (Gerrit)
Aadarsh Battula added 7 comments![]( "Open in Gerrit")
"""Discovers all trybots and target groups configuring the test suite."""As mentioned in the other review, please include docstrings with argument and return value information for any non-trivial functions/methods.
Marked as resolved.
if len(parts) >= 8 and parts[4] == 'try' and parts[6] == 'targets':This seems overly complicated. The `git grep` command is already filtering to the relevant files, so we shouldn't need to do any more checking?
Marked as resolved.
results.append({
'builder': builder_name,
'ci_only': is_ci_only,
})Nit: A named tuple or a dataclass is more fitting if the keys are fixed.
Marked as resolved.
f" {len(trybots_and_groups)} configured trybots{revision_msg}...")Single quotes since there aren't any single quotes in the string itself.
Marked as resolved.
Handle mocking for commonly used functions like this one in `setUp` instead of applying a decorator everywhere.
Marked as resolved.
'infra/config/generated/builders/try/android-code-coverage/targets/'
'chromium.coverage.json\n'
'infra/config/generated/builders/try/android-x86-code-coverage/'
'targets/chromium.coverage.json\n'),Use `textwrap.dedent` and a multiline string instead. Relevant here and elsewhere in this file.
Marked as resolved.
Shouldn't be necessary with `pyfakefs`.
- Brian Sheedy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Sheedy (Gerrit)
Brian Sheedy added 2 comments![]( "Open in Gerrit")
from typing import Any, Callable, Dict, List, NamedTuple, OptionalWe should be on a new enough Python version that `dict` and `list` support subscripts, so the `typing` imports should be unneeded. Is this related to this directory using an older version of pylint that doesn't support newer Python features?
from typing import Any, Callable, Dict, List, NamedTuple, OptionalSimilarly, we should support `| None` instead.
Related details
- Aadarsh Battula
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aadarsh Battula (Gerrit)
Aadarsh Battula added 2 comments![]( "Open in Gerrit")
from typing import Any, Callable, Dict, List, NamedTuple, OptionalWe should be on a new enough Python version that `dict` and `list` support subscripts, so the `typing` imports should be unneeded. Is this related to this directory using an older version of pylint that doesn't support newer Python features?
Python version: 3.11.9
Pylint version: 3.2.7
Just removed the typing imports for Dict and List.
from typing import Any, Callable, Dict, List, NamedTuple, OptionalSimilarly, we should support `| None` instead.
Got it, just updated it in the necessary function headers
Related details
- Brian Sheedy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Sheedy (Gerrit)
Brian Sheedy voted Code-Review+1![]( "Open in Gerrit")
- Aadarsh Battula
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aadarsh Battula (Gerrit)
Message from Aadarsh Battula![]( "Open in Gerrit")
- Brian Sheedy
- Prakhar Asthana
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Sheedy (Gerrit)
Brian Sheedy voted Code-Review+1![]( "Open in Gerrit")
- Aadarsh Battula
- Prakhar Asthana
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Prakhar Asthana (Gerrit)
Prakhar Asthana voted and added 1 comment![]( "Open in Gerrit")
Votes added by Prakhar Asthana
| Code-Review | +1 |
1 comment
def read_disk(src_root: pathlib.Path, rel_path: str) -> str | None:read_local
Related details
- Aadarsh Battula
- Brian Sheedy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aadarsh Battula (Gerrit)
Aadarsh Battula added 1 comment![]( "Open in Gerrit")
def read_disk(src_root: pathlib.Path, rel_path: str) -> str | None:read_local
Marked as resolved.
- Brian Sheedy
- Prakhar Asthana
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Sheedy (Gerrit)
Brian Sheedy voted Code-Review+1![]( "Open in Gerrit")
- Aadarsh Battula
- Prakhar Asthana
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |