With this tests will be easier to read and support.
Also have improved parameterized tests.
The reason why pytest was picked is because as my knowledge goes this is the most up to date and supported unit test framework in Python. The current parameterized tests state is primitive, and the parameterized library was last updated 2 years ago. pytest will help us adopt best practices that later perhaps we could spread to other unit tests in depot_tools.
Bug: 459690822
Change-Id: I67d16b56ff3c4dbd260ea0b07354ef766a6a6964
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7246550
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Alex Ovsienko <ovsienko@google.com>
Reviewed-by: Junji Watanabe <jwata@google.com>
On Windows, the command line has a character limit of 8191. When checking for commit objects in submodules, the `git ls-tree` command can become very long if many files are affected, potentially exceeding this limit.
This change modifies `CheckForCommitObjects` to check the estimated length of the `git ls-tree` command when running on Windows. If the command line with all affected files would exceed the limit, it falls back to using a recursive `ls-tree` (`-r`) instead of listing each file individually. This prevents command line overflow errors on Windows.
Change-Id: I6a340baefee57f5933473add0601a42ff1e61bb6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7254474
Auto-Submit: Josiah Kiehl <kiehl@google.com>
Commit-Queue: Josiah Kiehl <kiehl@google.com>
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
Significantly reduces the execution time of presubmit checks by
optimizing CheckForCommitObjects.
For CLs with fewer than 1000 affected files, run `git ls-tree` only on
specific files instead of scanning the full tree. This yields a ~70x
speedup (~0.97s -> ~0.01s) for typical CLs.
Change-Id: Ia8b89dbb14a5129ba79944282deba52a3558bdf2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7234371
Commit-Queue: Josiah Kiehl <kiehl@google.com>
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
This CL optimizes CheckForCommitObjects in two ways:
- Fast Path: If the raw `git ls-tree` output does not contain `160000` (gitlink mode), return early. This avoids parsing entirely for repositories without submodules at the cost of about 3ms.
- Iterative Parsing: For repositories with submodules, use an iterative find loop to locate gitlink entries instead of splitting the entire tree content.
On the Chromium repo:
- Speed: The parsing step is ~7.7x faster (0.04s vs 0.30s).
- Memory: Saves ~78 MB of memory per run by avoiding the creation of ~300,000 string objects during the split.
I also added tests now that it's more than a simple loop.
Change-Id: I903effc50aaedb9130772491ad38385b22477d58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7231149
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
Commit-Queue: Josiah Kiehl <kiehl@google.com>
ast.Str et al. have been deprecated since Py3.8 and have been
completely removed in 3.14. Replace their usage with ast.Constant.
This change should not have any functional impact, since
according to [1]:
> Changed in version 3.8: Class ast.Constant is now used for
> all constants.
> Deprecated since version 3.8: Old classes ast.Num, ast.Str,
> [...] instantiating them will return an instance of a
> different class.
[1] https://docs.python.org/3/library/ast.html
Bug: 40283283
Change-Id: I0ed8ef3910f921483bff118976f516c5a935e0fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7228507
Reviewed-by: Gavin Mak <gavinmak@google.com>
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
Commit-Queue: jj <jj@chromium.org>
Refactor GetCommentsSummary to group individual comments into threads. A
thread is considered unresolved if and only if its last comment is
unresolved.
This fixes a bug where a resolved thread (one that had been replied to
and the resolved box checked in the reply) would still appear with the
--unresolved flag, because the initial comment in the thread is
permanently marked as unresolved.
This change also introduces a CommentInfo dataclass to improve
ergonomics for handling comment data from the Gerrit API.
The display of comments have not been made thread-aware to reduce the
scope of this change.
R=agrieve@chromium.org
Bug: 463411949
Fixed: 463411949
Change-Id: I966f011140ee3874b54d8afbbe00caed7bbe14eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7200823
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
Auto-Submit: Peter Wen <wnwen@chromium.org>
Commit-Queue: Peter Wen <wnwen@chromium.org>
Since fetches involve multiple subprocess calls, any of which can be
slow, the per-subprocess caffeination strategy does not seem suitable --
the Mac might sleep as soon as the wake lock is dropped, before it
starts a new one. This instead implements a context manager to allow
caffeinating a scope.
To allow flag control, caffeinate.scope takes an argument that decides
whether or not it should actually do anything useful; it looks silly,
but the alternative is to interfere with flag parsing more or to require
users to write separate codepaths to decide whether to enter the context
manager scope or not; the "use the context manager in a mode where it
does not do anything" prevents this.
Bug: 462507017
Change-Id: Icc5bb9cadda30b5a120f112b10bf96ffd3b6550f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7183647
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Adam Norberg <norberg@google.com>
The diff hunk can omit the number of lines after the line position.
For example, the following hunks are all valid
- @@ -1,2 +1,3 @@
- @@ -1 +1,3 @@
- @@ -1,2 +1 @@
- @@ -1 +1 @@
However, the existing regex assumes that the number of lines is always
given. It wasn't a problem before crrev.com/c/6952890, because -U3 was
applied always. However, since it changed the context lines to -U0,
git diff started generating hunks without the number of lines, if
a given changed section contains only a single line.
This CL fixes the regex.
Bug: 453641840
Change-Id: I67f680bc1d8fa046e373b065e8ccbb6ce652eb3f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7158599
Auto-Submit: Scott Lee <ddoman@chromium.org>
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
Commit-Queue: Scott Lee <ddoman@chromium.org>
This CL implements ensure_authenticated() for GitCredsAuthenticator, and
adds ReAuth support.
This check performs best-effort credential checks before performing
expensive operations such as presubmit hooks. This allows us to fail fast when an ReAuth token is missing for a contributor subject to
ReAuth requirements.
If you suspect this CL has caused a breakage on CL upload, please
revert.
Bug: 451651615
Change-Id: I42f3ffe3c830ed7bd2322f6cf2ad8adbba7e7251
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7036772
Reviewed-by: Allen Li <ayatane@chromium.org>
Reviewed-by: Scott Lee <ddoman@chromium.org>
Commit-Queue: Jiewei Qian <qjw@chromium.org>
This CL makes it possible for ensure_authenticated to check whether
the current credential would satisfy ReAuth requirement.
Right now, only GitCredsAuthenticator performs additional ReAuth
check. For other Authenticator, either ReAuth is not relevant
(e.g. SSO), or not supported or implied (e.g. .gitcookies).
This CL does nothing on its own, because ensure_autheticated with
ReAuth is opt-in. Subsequent CL will update callsites to pass in
ReAuthContext to make the check effective.
Bug: 451651615
Change-Id: Idb1e70c52aaa844e672331fad8d462a2a7577d45
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7104363
Commit-Queue: Jiewei Qian <qjw@chromium.org>
Reviewed-by: Scott Lee <ddoman@chromium.org>
Reviewed-by: Allen Li <ayatane@chromium.org>
This CL refactors _test_gerrit_ensure_authenticated_common by splitting
it into:
- The actual mock and common code path
- Test case specific setup (e.g. scm.GIT.SetConfig)
In addition, fixes a few issues with the existing code:
- _Authenticator.get() is now mocked correctly, so a CookiesAuthenticator
is returned in relevant tests. Previously it returned the "production"
ChainedAuthenticator, which could call other authenticators (such as
GitCredsAuthenticator based on the runtime environment)
- Adds mock patch cleanup functions. Previously, cleanup functions
aren't registered, so the mock leaks and got overwritten by other test
cases.
- Rewrite 'Bearer' in Authorization header check to be more robust.
Previously, the test case raise a KeyError exception when the
Authorization header isn't set at all in request headers.
This is a preparation CL for implementing ensure_authenticated() method
in ChainedAuthenticator and GitCredsAuthenticator in future CLs.
Bug: 348024314
Change-Id: I28a7fabbc6cf2dc33dccc1339b89d20a22dc12ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7082265
Commit-Queue: Jiewei Qian <qjw@chromium.org>
Reviewed-by: Scott Lee <ddoman@chromium.org>
This CL implements attempt_authenticate_with_reauth in LuciContextAuthenticator according to the base _Authenticator's
interface definition.
This allows LuciContextAuthenticator to be used where an action that needs to meet ReAuth requirement (instead of raising an exception
saying "ReAuth
is required").
Luci context access tokens should already satisfy ReAuth requirement.
Bug: 442666611
Change-Id: I2e4d43a75b230932fc779c805f75b5828c9d0980
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6967915
Reviewed-by: Allen Li <ayatane@chromium.org>
Commit-Queue: Jiewei Qian <qjw@chromium.org>
Reviewed-by: Scott Lee <ddoman@chromium.org>
This CL implements attempt_authenticate_with_reauth in GceAuthenticator
according to the base _Authenticator's interface definition.
This allows GceAuthenticator to be used where an action that needs to
meet ReAuth requirement (instead of raising an exception saying "ReAuth
is required").
GceAuthenticator's credential already satisfies ReAuth requirement.
ReAuth is satisfied if the GCE bot is trusted (controlled by Gerrit
server side config).
Bug:442666611
Change-Id: I9a801e3fd6ab9fb446e7842a93bcd9ee1ff953c7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6965527
Reviewed-by: Allen Li <ayatane@chromium.org>
Commit-Queue: Jiewei Qian <qjw@chromium.org>
Reviewed-by: Scott Lee <ddoman@chromium.org>
