Skip to content

test(release): cover Windows research installer collection - #1438

Open
TimeToBuildBob wants to merge 1 commit into
ActivityWatch:masterfrom
TimeToBuildBob:test/research-windows-packaging-0108
Open

test(release): cover Windows research installer collection#1438
TimeToBuildBob wants to merge 1 commit into
ActivityWatch:masterfrom
TimeToBuildBob:test/research-windows-packaging-0108

Conversation

@TimeToBuildBob

Copy link
Copy Markdown
Contributor

The Research Edition Windows builds failed after the patcher renamed Inno Setup outputs while packaging still expected activitywatch-setup.exe (#1436). Linux PR checks never exercised that consumer.

Extract the existing glob/count/move step into collect-setup.sh, called by build_setup and exercised by the existing Ubuntu research smoke job. Tests derive standard and research Qt/Tauri names from the real .iss sources, verify the packaging and release upload globs, and check that collection rejects missing, duplicate, or directory inputs without moving files. No additional Windows build or signing job is needed.

Validation:

  • Profile/packaging suite: 36 passed on Python 3.9, matching the smoke job.
  • Full script suite: 82 passed; global/system Git configuration isolated for temporary Git fixture repositories.
  • Real-tree patch checks: 44 Qt and 23 Tauri sites verified; Bash syntax and helper ShellCheck clean.
  • Mutation checks: restoring the hardcoded filename fails five cases; removing the count guard fails the duplicate-input case.
  • Independent review: no blocking findings.

This covers filename contracts and artifact collection; compiling/installing Windows binaries remains covered by the existing build matrix. The follow-up does not gate the Research Edition release recut.

@greptile-apps

greptile-apps Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge; no actionable correctness, security, or repository-rule violations were identified.

Summary

  • Uses the actual patched Qt and Tauri installer definitions to cover standard and research names.
  • Rejects missing, duplicate, and non-file installer inputs before moving anything.
  • Confirms package upload and release globs accept the resulting versioned artifacts.
  • Expands the existing Ubuntu smoke job without adding another Windows build.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    ISS[Qt or Tauri .iss] -->|standard or research patch| ISCC[Inno Setup build]
    ISCC --> Produced[dist/activitywatch*-setup.exe]
    Produced --> Collector[collect-setup.sh]
    Collector -->|require exactly one regular file| Versioned[Versioned Windows setup artifact]
    Versioned --> Upload[Platform artifact upload]
    Upload --> Release[Draft release asset collection]
    Tests[Ubuntu research smoke tests] -. verify naming and globs .-> ISS
    Tests -. exercise valid and invalid inputs .-> Collector
    Tests -. verify publication patterns .-> Release
Loading

@TimeToBuildBob

TimeToBuildBob commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI code review

Safe to merge — no P0/P1 findings on latest review

Updated after inline dispositions on finding threads — this is the current state; the verdict below is frozen at review time and is kept as the historical record of that pass.

Finding disposition
Finding Severity State
scripts/tests/test_patch_research_edition_profile.py:408 P2 rejected

This PR adds tests that derive standard/research Qt and Tauri installer names from real .iss sources, verify packaging and release upload globs, and validate collection behavior on missing, duplicate, or directory inputs. It also updates the packaging script to ensure submodule pins are checked on PRs and release branches. The changes appear to improve coverage for Windows installer name collisions and release upload consistency.

Needs a look — P2 only

Confidence 4/5

⚠️ 1 of 4 changed files were not fully read. Claims that an identifier is never assigned, used, or called are discarded unless verification names only fully preloaded files. This coverage describes the bounded context preload; the diff was reviewed.

1 finding · ⚠️ 1 P2

⚠️ P2 mediumscripts/tests/test_patch_research_edition_profile.py:408

In test_patch_research_edition_profile.py, the test asserts files is truthy but then indexes files[1] without ensuring it has at least two elements. If files is a single-line string or a one-element list, this raises IndexError instead of a descriptive assertion failure, making the test fail with an unclear error. The test should check that files has at least two lines or parse the block robustly before accessing files[1].

assert len(files) > 1, "expected at least two lines in the asset files block"
    release_globs = [line.strip() for line in files[1].splitlines()]

How this was verified: The assumption that files always contains at least two entries is not enforced by the prior assertion, so a valid but minimal block could trigger an unhandled IndexError. This makes the test flaky or gives misleading failure output.

Consensus: 2/3 passes agreed — pass 3 looked and disagreed
Distinct keys: 1 (general)

Files changed (2) — the diff as I read it
  • scripts/package/package-all.sh — Modified script to package all build artifacts.
  • scripts/tests/test_patch_research_edition_profile.py — Test file for the research edition profile patch.

Also changed, not described above: .github/workflows/release.yml, scripts/package/collect-setup.sh

Reviewed c6e8708660c8 · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 238s · about this reviewer

Maintainer commands

@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.

Comment thread scripts/tests/test_patch_research_edition_profile.py
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

All CI checks are green and Greptile is 5/5 with no actionable findings. The only inline AI-review finding was rejected as a false positive: files is a re.Match, and [1] accesses its mandatory capture group.

Self-merge is not eligible because this account has pull-only access to ActivityWatch/activitywatch and the PR touches workflow/release infrastructure. No additional code change is available; ready for maintainer review and squash merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant