JMR-dev/LibreMediaConverter
1
0
Fork 0
Code Issues 16 Pull Requests Actions Packages Projects Releases Wiki Activity

Three comments still say the advisory API 37 job runs two tests; it runs three #81

New Issue
Closed
opened 2026-08-25 02:24:22 +00:00 by JMR-dev · 1 comment
JMR-dev commented 2026-08-25 02:24:22 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Rescoped 2026-08-25. This was originally filed as "rename the advisory job". That was the wrong fix — see the comment below. What is actually wrong is three statements of fact.

A third test joined the advisory job, and three places still describe two

#80 added SafPickerRoundTripTest.thePickedInputSurvivesARealRotation to the
@FailsOnEmulatorApi37 marker, because a real rotation takes the framework down on android-37.0
(INSTRUMENTATION_ABORTED: System has crashed). Three tests now carry the marker — two in
Media3EngineTest, one in SafPickerRoundTripTest.

Three claims were true when there were two and are false now. Locate them by their text, not by
line number — these have already moved once:

1. .github/workflows/status_check.yml, in the gating API 37 matrix entry:

notAnnotation removes the two tests that do not pass on this image

2. CLAUDE.md, in "Instrumented tests: where they actually run":

do not read a green run as evidence those two tests pass

3. .github/workflows/status_check.yml, above the advisory job — and this one is worse than a
count, because it is the stated justification for the job's name:

It is named for WHAT IT RUNS, deliberately. Both tests drive a full H.264 -> H.265 hardware
transcode through Media3Engine

The rotation test drives no transcode. So the comment does not merely miscount — it asserts an
invariant the code no longer satisfies, and that invariant is the whole argument for the name.

The fix

Correct all three to describe three tests, and rewrite #3 so it stops claiming every test in the job
is a hardware transcode. The honest description of the job's contents is "the tests that cannot pass
on the API 37 emulator image", whatever their subject.

Do not rename the job. That was this ticket's original proposal and it is withdrawn:

  • The name was chosen deliberately and the reasoning sits next to it — "without renaming a check
    that people have already learned to look for."     Undoing that needs a better argument than tidiness.
  • The job is continue-on-error, red on every PR by design, and not a required context
    (verified against ruleset 21117412, whose eight contexts do not include it). Nobody triages from
    it, so the name costs little.
  • Once #3 stops overclaiming, the name is approximate rather than false, which is a different and
    much cheaper problem.

Acceptance

The three quoted strings no longer appear, and what replaces them matches the marker's actual usage.
Verify by counting, not by reading:

grep -rn "@FailsOnEmulatorApi37" app/src/androidTest --include='*.kt' | grep -c FailsOn

must equal the number every corrected comment states. That command is the check to re-run the next
time a test joins or leaves the marker — which is the event that broke this twice.

Related, and NOT fixed here

The advisory job is permanently red, so a new failure joining it is invisible. Flagged during the
#56 review and still without a detector; a name or a comment cannot fix it. Its own ticket if it is
worth one.

_Rescoped 2026-08-25. This was originally filed as "rename the advisory job". **That was the wrong fix** — see the comment below. What is actually wrong is three statements of fact._ ### A third test joined the advisory job, and three places still describe two `#80` added `SafPickerRoundTripTest.thePickedInputSurvivesARealRotation` to the `@FailsOnEmulatorApi37` marker, because a real rotation takes the framework down on `android-37.0` (`INSTRUMENTATION_ABORTED: System has crashed`). Three tests now carry the marker — two in `Media3EngineTest`, one in `SafPickerRoundTripTest`. Three claims were true when there were two and are false now. **Locate them by their text, not by line number** — these have already moved once: **1. `.github/workflows/status_check.yml`**, in the gating API 37 matrix entry: > notAnnotation removes **the two tests** that do not pass on this image **2. `CLAUDE.md`**, in "Instrumented tests: where they actually run": > do not read a green run as evidence **those two tests** pass **3. `.github/workflows/status_check.yml`**, above the advisory job — and this one is worse than a count, because it is the stated justification for the job's name: > It is named for WHAT IT RUNS, deliberately. **Both tests** drive a full H.264 -> H.265 hardware > transcode through Media3Engine The rotation test drives no transcode. So the comment does not merely miscount — **it asserts an invariant the code no longer satisfies**, and that invariant is the whole argument for the name. ### The fix Correct all three to describe three tests, and rewrite #3 so it stops claiming every test in the job is a hardware transcode. The honest description of the job's contents is "the tests that cannot pass on the API 37 emulator image", whatever their subject. **Do not rename the job.** That was this ticket's original proposal and it is withdrawn: - The name was chosen deliberately and the reasoning sits next to it — *"without renaming a check that people have already learned to look for."* Undoing that needs a better argument than tidiness. - The job is `continue-on-error`, **red on every PR by design**, and **not** a required context (verified against ruleset `21117412`, whose eight contexts do not include it). Nobody triages from it, so the name costs little. - Once #3 stops overclaiming, the name is approximate rather than false, which is a different and much cheaper problem. ### Acceptance The three quoted strings no longer appear, and what replaces them matches the marker's actual usage. **Verify by counting, not by reading:** ``` grep -rn "@FailsOnEmulatorApi37" app/src/androidTest --include='*.kt' | grep -c FailsOn ``` must equal the number every corrected comment states. That command is the check to re-run the next time a test joins or leaves the marker — which is the event that broke this twice. ### Related, and NOT fixed here **The advisory job is permanently red, so a new failure joining it is invisible.** Flagged during the #56 review and still without a detector; a name or a comment cannot fix it. Its own ticket if it is worth one.
JMR-dev commented 2026-08-25 02:54:05 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Rescoped. The original proposal — rename the advisory job — is withdrawn, and it is worth saying why rather than quietly editing the body.

I filed this after #80 pointed out that E2E API 37 Media3 hardware transcode (advisory) now runs a test that is neither Media3 nor a hardware transcode. That observation is correct. The conclusion I drew from it was not.

What I had not checked when I filed it: the workflow comment directly above the job says the name was chosen on purpose —

It is named for WHAT IT RUNS, deliberately. … the current theory about why they fail is in the next paragraph, where it can be corrected without renaming a check that people have already learned to look for.

So a previous pass considered this exact question and chose stability. Renaming now would undo a decision that has its reasoning written beside it, on grounds no stronger than tidiness.

And the cost of the mismatch is small. The job is continue-on-error, red on every PR by design, and not among ruleset 21117412's eight required contexts. Nobody diagnoses from it; anyone who opens it sees the failing test named in the log immediately.

What is actually wrong is narrower and more serious than a name: three comments state a count that is now false, and one of them justifies the name by asserting every test in the job is a hardware transcode — an invariant SafPickerRoundTripTest.thePickedInputSurvivesARealRotation breaks. That is a false statement in the project's own instructions, which is the R14/R15/R20/R25 class this repo has already paid for four times. Fixing it makes the name approximate instead of false, which is the cheap and honest outcome.

Same defect, one layer down: the thing worth correcting was the claim, not the label.

Rescoped. The original proposal — rename the advisory job — is **withdrawn**, and it is worth saying why rather than quietly editing the body. I filed this after #80 pointed out that `E2E API 37 Media3 hardware transcode (advisory)` now runs a test that is neither Media3 nor a hardware transcode. That observation is correct. The conclusion I drew from it was not. **What I had not checked when I filed it:** the workflow comment directly above the job says the name was chosen on purpose — > It is named for WHAT IT RUNS, deliberately. … the current theory about why they fail is in the next paragraph, where it can be corrected **without renaming a check that people have already learned to look for.** So a previous pass considered this exact question and chose stability. Renaming now would undo a decision that has its reasoning written beside it, on grounds no stronger than tidiness. **And the cost of the mismatch is small.** The job is `continue-on-error`, red on every PR by design, and not among ruleset `21117412`'s eight required contexts. Nobody diagnoses from it; anyone who opens it sees the failing test named in the log immediately. **What is actually wrong is narrower and more serious than a name:** three comments state a count that is now false, and one of them justifies the name by asserting every test in the job is a hardware transcode — an invariant `SafPickerRoundTripTest.thePickedInputSurvivesARealRotation` breaks. That is a false statement in the project's own instructions, which is the R14/R15/R20/R25 class this repo has already paid for four times. Fixing it makes the name approximate instead of false, which is the cheap and honest outcome. Same defect, one layer down: the thing worth correcting was the claim, not the label.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
above-cut
Worked autonomously overnight: local, JVM-verifiable, no product decision
 accessibility
Barrier affecting people with disabilities
 backlog
Held for manual review: product/UX call, CI/workflow, hardware, or unverifiable here
 bug
Something isn't working
 confirmed
Reviewer demonstrated the defect
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 plausible
Reviewer could not fully demonstrate it; treat as unproven
 question
Further information is requested
 sev:high
High severity
 sev:low
Low severity
 sev:medium
Medium severity
 wontfix
This will not be worked on
No labels confirmed documentation sev:low
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
JMR-dev
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: JMR-dev/LibreMediaConverter#81