Skip to content

IGNORE run dll checks - #1618

Open
ben-c-at-moz wants to merge 7 commits into
mainfrom
ben/dll-new-tests
Open

IGNORE run dll checks#1618
ben-c-at-moz wants to merge 7 commits into
mainfrom
ben/dll-new-tests

Conversation

@ben-c-at-moz

@ben-c-at-moz ben-c-at-moz commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Relevant Links

Bugzilla: link

Description of Code / Doc Changes

  • Create script that obtains and runs DLL tests
  • Connect that test to the end of the Windows workflow for scheduled executions

Process Changes Required

Mark the relevant boxes, delete irrelevant lines.

  • Adds a dependency (rerun uv sync)
  • Changes scheduled Beta / DevEdition / RC

Comments or Future Work

Currently we only test x64, not sure whether we can even run on x32. We also can't report it to TestRail as the DLL tests don't live there.

Workflow Checklist

  • Reviewers have been requested.
  • Code has been linted and formatted.

Thank you!

Comment thread dll_check/check_dll_interceptors.py Fixed
Comment thread dll_check/check_dll_interceptors.py Fixed
@ben-c-at-moz ben-c-at-moz mentioned this pull request Sep 2, 2026
10 tasks
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Review: Spike test - DLL Interceptors

This is an interesting spike, but there are several bugs that would prevent it from running correctly in CI.

Critical Issues

Missing FX_DOWNLOAD_URL env var in workflow (would crash immediately): The CI step only sets FX_EXECUTABLE, but the script calls os.getenv("FX_DOWNLOAD_URL").rsplit(...). Since the env var is not set, this returns None and raises AttributeError before any work is done.

Silent no-op on missing jobs (line 34-35): if not jobs_list: return None — if Treeherder returns no jobs, the function exits silently and all assertions are skipped. The CI step would show green with zero actual testing done. This TODO needs to be resolved before this lands.

Architecture mismatch: SEARCH_STRING hardcodes x86 (32-bit) but the entry point only calls main(64). The Treeherder search and the job type fetched (build-win64-shippable/opt) are inconsistent — the search may return wrong or no results.

Minor Issues

  • Comment/code mismatch: Line 9 says env var is FX_DOWNLOAD_LINK, code uses FX_DOWNLOAD_URL.
  • Unused import: from time import sleep is never used.
  • Unused constant: TREEHERDER_LINK is defined but never referenced.
  • pipenv run vs uv run: All other workflow steps use uv run/uv sync, but this new step uses pipenv run. The dependency was added to pyproject.toml/uv.lock so uv run would be the consistent choice.
  • Old dependency: treeherder-client==5.0.0 (2019) depends on six, which is a deprecated Py2/3 compat library. Worth checking if there is a more current alternative or if the API used here is still valid.
  • shell=True with list arg: On Windows, check_output([".\TestDllInterceptor.exe"], shell=True) has subtle behavior — the list is joined differently than on Unix. Passing a plain string would be more predictable.

Comment thread dll_check/check_dll_interceptors.py
Comment thread dll_check/check_dll_interceptors.py Outdated
Comment thread dll_check/check_dll_interceptors.py Outdated
Comment thread dll_check/check_dll_interceptors.py Outdated
Comment thread .github/workflows/main.yml Outdated
Comment thread dll_check/check_dll_interceptors.py Outdated
Comment thread dll_check/check_dll_interceptors.py
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Good spike concept. The github-actions bot caught several issues from a prior iteration — here's what remains in the current diff:

Still broken - will crash in CI:

  • FX_DOWNLOAD_URL is never set in the workflow step (only FX_EXECUTABLE is). os.getenv("FX_DOWNLOAD_URL") returns None, and .rsplit("/", 1) immediately raises AttributeError.

Still incomplete:

  • if not jobs_list: return None # replace with raise — silent no-op means CI shows green with zero assertions tested.

Architecture mismatch:

  • SEARCH_STRING hardcodes x86/build-win32-shippable/opt (32-bit) but the entry point calls main(64), so the Treeherder query likely returns no results or wrong jobs.

Minor:

  • TREEHERDER_LINK constant is defined but never used.
  • The top comment says FX_DOWNLOAD_LINK but the code reads FX_DOWNLOAD_URL.
  • Since this is a spike, consider adding continue-on-error: true to the workflow step so a failure here doesn't block the rest of CI.

Comment thread dll_check/check_dll_interceptors.py
Comment thread dll_check/check_dll_interceptors.py Outdated
@ben-c-at-moz ben-c-at-moz added the skip-claude-review Skip the Claude Code Reviewer workflow on this PR label Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-claude-review Skip the Claude Code Reviewer workflow on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants