Skip to content

Add --link_existing_gcs_logs for the Results Uploader #42

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.

Sign up for GitHub

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

Open
xianyuanjia wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add --link_existing_gcs_logs for the Results Uploader #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 34 additions & 15 deletions src/results_uploader/results_uploader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,13 @@ def _parse_args(argv: list[str] | None) -> argparse.Namespace:
'Timeout (in seconds) to upload each file to GCS. '
f'Default: {_GCS_DEFAULT_TIMEOUT_SECS} seconds.'),
)
parser.add_argument(
'--link_existing_gcs_logs',
action='store_true',
help='Associates the invocation with a GCS directory that already '
'contains the uploaded Mobly logs, as specified by --gcs_dir. '
'The Mobly logs must be exactly the same as in logs_dir.'
)
parser.add_argument(
'--test_title',
help='Custom test title to display in the result UI.'
Expand Down Expand Up @@ -391,6 +398,16 @@ def _get_test_result_info_from_test_xml(
return test_result_info


def _get_file_paths_recursively(dir_path: pathlib.Path) -> list[str]:
"""Recursively gets all file paths in a directory."""
glob = dir_path.rglob('*')
return [
str(path.relative_to(dir_path).as_posix())
for path in glob
if path.is_file()
]


def _upload_dir_to_gcs(
src_dir: pathlib.Path, gcs_bucket: str, gcs_dir: str, timeout: int
) -> list[str]:
Expand All @@ -403,12 +420,7 @@ def _upload_dir_to_gcs(

bucket_obj = storage.Client().bucket(gcs_bucket)

glob = src_dir.rglob('*')
file_paths = [
str(path.relative_to(src_dir).as_posix())
for path in glob
if path.is_file()
]
file_paths = _get_file_paths_recursively(src_dir)

logging.info(
'Uploading %s files from %s to Cloud Storage directory %s/%s...',
Expand Down Expand Up @@ -482,11 +494,12 @@ def _add_resultstore_target(
file_paths: list[str],
status: _Status,
target_id: str | None,
assign_undeclared_outputs: bool = False,
) -> None:
"""Calls the Resultstore Upload API to create and populate a new target."""
client.create_target(target_id)
client.create_configured_target()
client.create_action(gcs_bucket, gcs_dir, file_paths)
client.create_action(gcs_bucket, gcs_dir, file_paths, assign_undeclared_outputs)
client.merge_configured_target(status)
client.finalize_configured_target()
client.merge_target(status)
Expand Down Expand Up @@ -587,7 +600,7 @@ def main(argv: list[str] | None = None) -> None:
target_statuses = []
try:
for idx, mobly_dir in enumerate(mobly_dirs):
gcs_dir = gcs_base_dir.joinpath(f'dir{idx}')
gcs_dir = gcs_base_dir.joinpath(f'dir{idx}') if not args.link_existing_gcs_logs else gcs_base_dir
Copy link

@gemini-code-assist gemini-code-assist bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This logic could lead to incorrect behavior if args.link_existing_gcs_logs is true and there are multiple Mobly directories found (len(mobly_dirs) > 1). In that case, all targets will point to the same gcs_base_dir, but will be associated with files from different local mobly_dir directories. This is likely not the intended behavior.

To prevent this, I recommend adding a check earlier in the main function to ensure that if link_existing_gcs_logs is used, there is at most one Mobly directory to process. For example, after mobly_dirs is populated:

if args.link_existing_gcs_logs and len(mobly_dirs) > 1:
    logging.error(
        '--link_existing_gcs_logs can only be used with a single Mobly result directory found within logs_dir.'
    )
    exit(1)

if args.no_convert_result:
# Determine the final status based on the test.xml
test_xml = ElementTree.parse(mobly_dir.joinpath(_TEST_XML))
Expand All @@ -610,13 +623,18 @@ def main(argv: list[str] | None = None) -> None:
converted_dir, gcs_bucket, gcs_dir.as_posix(),
args.gcs_upload_timeout
)
# Upload remaining logs to undeclared_outputs/ subdirectory
gcs_files += _upload_dir_to_gcs(
dir_to_upload,
gcs_bucket,
gcs_dir.joinpath(_UNDECLARED_OUTPUTS).as_posix(),
args.gcs_upload_timeout
)
if args.link_existing_gcs_logs:
# Use already uploaded Mobly logs in GCS
logging.info('Using existing Mobly logs in GCS dir: %s', gcs_dir)
gcs_files += [f'{gcs_dir}/{path}' for path in _get_file_paths_recursively(dir_to_upload)]
Copy link

@gemini-code-assist gemini-code-assist bot Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The path construction f'{gcs_dir}/{path}' is not platform-independent. On Windows, str(gcs_dir) will use backslashes as separators, which will result in mixed-separator GCS paths (e.g., my\gcs/dir/some/file.txt).

To ensure consistent behavior across platforms, you should use .as_posix() to get a POSIX-style path string, similar to how it's used elsewhere in this file.

Suggested change
gcs_files += [f'{gcs_dir}/{path}' for path in _get_file_paths_recursively(dir_to_upload)]
gcs_files += [f'{gcs_dir.as_posix()}/{path}' for path in _get_file_paths_recursively(dir_to_upload)]

else:
# Upload Mobly logs to undeclared_outputs/ subdirectory
gcs_files += _upload_dir_to_gcs(
dir_to_upload,
gcs_bucket,
gcs_dir.joinpath(_UNDECLARED_OUTPUTS).as_posix(),
args.gcs_upload_timeout
)
# Override target_id as needed
if args.cts:
test_result_info.target_id = mobly_dir.parent.name
Expand All @@ -630,6 +648,7 @@ def main(argv: list[str] | None = None) -> None:
gcs_files,
test_result_info.status,
test_result_info.target_id,
args.link_existing_gcs_logs,
)
target_statuses.append(test_result_info.status)
finally:
Expand Down
10 changes: 9 additions & 1 deletion src/results_uploader/resultstore_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@

_PACKAGE_NAME = 'mobly-android-partner-tools'

_RESULTSTORE_RESERVED_FILES = ('test.log', 'test.xml')


@dataclasses.dataclass
class Timing:
Expand Down Expand Up @@ -276,7 +278,8 @@ def create_configured_target(self) -> None:
logging.debug('invocations.targets.configuredTargets.create: %s', res)

def create_action(
self, gcs_bucket: str, gcs_dir: str, artifacts: list[str]
self, gcs_bucket: str, gcs_dir: str, artifacts: list[str],
assign_undeclared_outputs: bool = False
) -> str:
"""Creates an action.

Expand All @@ -285,6 +288,9 @@ def create_action(
gcs_dir: Base directory of the artifacts in the GCS bucket.
artifacts: List of paths (relative to gcs_bucket) to the test
artifacts.
assign_undeclared_outputs: Whether to assign artifacts to
undeclared_outputs. Special files for Resultstore, including
test.log and test.xml, would be skipped.

Returns:
The action ID.
Expand All @@ -295,6 +301,8 @@ def create_action(
files = []
for path in artifacts:
uid = str(pathlib.PurePosixPath(path).relative_to(gcs_dir))
if assign_undeclared_outputs and uid not in _RESULTSTORE_RESERVED_FILES:
uid = 'undeclared_outputs/' + uid
uri = f'gs://{gcs_bucket}/{path}'
files.append({'uid': uid, 'uri': uri})
action = {
Expand Down
Loading