Compare commits
53
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
c6b581ab5a | ||
|
|
34641df19d | ||
|
|
71141b5734 | ||
|
|
0f41bc3f6b | ||
|
|
4f1a007b58 | ||
|
|
bf78e06969 | ||
|
|
93ebfa6b4a | ||
|
|
d94906ef42 | ||
|
|
959bd8be13 | ||
|
|
b93ef79931 | ||
|
|
d739b425c0 | ||
|
|
cd77aceff4 | ||
|
|
febd141bea | ||
|
|
3d8b89bfab | ||
|
|
c4bb7d4d2d | ||
|
|
3599307040 | ||
|
|
3f731d8ea7 | ||
|
|
cc424dd08f | ||
|
|
b49295d2bf | ||
|
|
25aac95db9 | ||
|
|
dce516224c | ||
|
|
2b7520061b | ||
|
|
4e46eb99f6 | ||
|
|
62040b2161 | ||
|
|
83b557409e | ||
|
|
0eb00d2003 | ||
|
|
c887af0d83 | ||
|
|
2e0c6737d6 | ||
|
|
68f841e84f | ||
|
|
b53f326f9e | ||
|
|
85461943d6 | ||
|
|
238142d9cc | ||
|
|
9c809d4e16 | ||
|
|
bf2214a549 | ||
|
|
989069207e | ||
|
|
72ff7adfcc | ||
|
|
994ea8a3dd | ||
|
|
3e9528454c | ||
|
|
0702916229 | ||
|
|
3fd34c24a6 | ||
|
|
d01a46a708 | ||
|
|
3806641cb2 | ||
|
|
5f9498150c | ||
|
|
8d8703ab49 | ||
|
|
27b7654418 | ||
|
|
4a8e30099e | ||
|
|
e7d84cc69f | ||
|
|
a8b494b846 | ||
|
|
865a4a7c8e | ||
|
|
e856679395 | ||
|
|
49c483d877 | ||
|
|
1b220856ab | ||
|
|
d37c391c60 |
Executable
+193
@@ -0,0 +1,193 @@
|
||||
#!/usr/bin/env bash
|
||||
#
|
||||
# Exercises e2e-report-shape.sh's baseline counter against fixture source, with no emulator and
|
||||
# no CI run. Run it directly:
|
||||
#
|
||||
# .github/scripts/e2e-report-shape-test.sh
|
||||
#
|
||||
# WHY THIS CAN EXIST AT ALL: the counter is a pure function of the working tree. It greps
|
||||
# `app/src/androidTest` for `@FailsOnEmulatorApi37` and compares the total against the number
|
||||
# committed in FailsOnEmulatorApi37.kt. Nothing about that needs a device, which is the whole
|
||||
# reason #120 could be measured rather than argued about.
|
||||
#
|
||||
# WHY A THROWAWAY REPO ROOT rather than a knob on the script. The report finds its own root from
|
||||
# `BASH_SOURCE`, so a copy of it placed at `<root>/.github/scripts/` reads `<root>/app/src/...`.
|
||||
# Building that root is three mkdirs and costs the shipped script nothing:
|
||||
#
|
||||
# - the REAL script is what runs, byte for byte, so reverting the matcher reddens this test
|
||||
# rather than a testing-only code path beside it;
|
||||
# - no environment variable exists that could point the LIVE count somewhere else, which is
|
||||
# the failure mode #83 built the baseline check to prevent in the first place;
|
||||
# - XML_DIR resolves inside the throwaway root, so a stale app/build/outputs left by a real
|
||||
# run on a developer machine cannot leak into the numbers here.
|
||||
#
|
||||
# WHAT IT GUARDS (#120). The old matcher looked for the string anywhere on any line, so a KDoc
|
||||
# saying `Deliberately not @FailsOnEmulatorApi37` counted as a marked test and every PR got a
|
||||
# deviation notice that was wrong. The obvious repair -- count only lines that are nothing but
|
||||
# the annotation -- silently stops counting `@FailsOnEmulatorApi37 @Test`, which is legal Kotlin,
|
||||
# and undercounting is the direction that hides a genuine new marker. The fixture carries every
|
||||
# shape at once -- three that count and three that must not, enumerated in its own header -- so
|
||||
# both mistakes fail here instead of on a PR: against testdata/marker-shapes the old matcher says
|
||||
# 5, own-line-only says 2, and the shipped one 3.
|
||||
#
|
||||
# NOT WIRED INTO CI, deliberately and as a known gap. Adding a step to the Static analysis job
|
||||
# would add a new way for a gating job to go red, and #120 was explicit that nothing about it may
|
||||
# change any job's status or the pass/fail rules. shellcheck still covers this file, since that
|
||||
# step reads `git ls-files '*.sh'` rather than a fixed list.
|
||||
set -uo pipefail
|
||||
|
||||
SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)"
|
||||
REPORT="$SCRIPT_DIR/e2e-report-shape.sh"
|
||||
FIXTURE_DIR="$SCRIPT_DIR/testdata/marker-shapes"
|
||||
FIXTURE="$FIXTURE_DIR/MarkerShapes.kt"
|
||||
|
||||
TMP="$(mktemp -d)"
|
||||
# Single quotes: the path is expanded when the trap fires, not when it is set.
|
||||
trap 'rm -rf -- "$TMP"' EXIT
|
||||
|
||||
failures=0
|
||||
|
||||
pass() { printf 'ok %s\n' "$1"; }
|
||||
|
||||
fail() {
|
||||
failures=$((failures + 1))
|
||||
printf 'FAIL %s\n' "$1"
|
||||
shift
|
||||
printf ' %s\n' "$@"
|
||||
}
|
||||
|
||||
# assert_contains <name> <haystack> <needle>
|
||||
# `case` rather than grep: the strings being matched carry backticks and an em dash, and this
|
||||
# way neither the shell nor a regex engine gets an opinion about them.
|
||||
assert_contains() {
|
||||
case "$2" in
|
||||
*"$3"*) pass "$1" ;;
|
||||
*) fail "$1" "wanted to find: $3" "in:" "$2" ;;
|
||||
esac
|
||||
}
|
||||
|
||||
assert_absent() {
|
||||
case "$2" in
|
||||
*"$3"*) fail "$1" "did NOT want to find: $3" "in:" "$2" ;;
|
||||
*) pass "$1" ;;
|
||||
esac
|
||||
}
|
||||
|
||||
# make_root <marked-tree-dir> <baseline-number>
|
||||
# Assembles a throwaway repo root around the given tree and prints its path.
|
||||
make_root() {
|
||||
local tree="$1" baseline="$2" root testdir
|
||||
root="$(mktemp -d "$TMP/root.XXXXXX")"
|
||||
testdir="$root/app/src/androidTest/java/org/libremediaconverter"
|
||||
mkdir -p "$root/.github/scripts" "$testdir"
|
||||
cp -- "$REPORT" "$root/.github/scripts/"
|
||||
cp -- "$tree"/*.kt "$testdir/"
|
||||
|
||||
# The synthetic stand-in for the committed baseline. Its KDoc names the marker the way the real
|
||||
# file does -- in brackets, never with an `@` -- because the real file lives inside the tree
|
||||
# being counted, so an `@` spelling here would add a phantom to every number below.
|
||||
cat > "$testdir/FailsOnEmulatorApi37.kt" <<EOF
|
||||
package org.libremediaconverter
|
||||
|
||||
/** Stand-in for the real marker file. Only [FAILS_ON_EMULATOR_API37_BASELINE] is read. */
|
||||
const val FAILS_ON_EMULATOR_API37_BASELINE = $baseline
|
||||
EOF
|
||||
|
||||
# A clean, untruncated run of exactly <baseline> tests, all failing -- which is what the
|
||||
# advisory leg looks like when nothing has drifted. It leaves the marked count as the only
|
||||
# field that can deviate, so every assertion below is about the thing under test.
|
||||
cat > "$root/gradle.log" <<EOF
|
||||
> Task :app:connectedDebugAndroidTest
|
||||
Starting $baseline tests on test(AVD) - 16
|
||||
There was $baseline failure(s).
|
||||
EOF
|
||||
|
||||
printf '%s\n' "$root"
|
||||
}
|
||||
|
||||
# run_report <root> -- stdout of the real script; its summary lands in <root>/summary.md.
|
||||
run_report() {
|
||||
E2E_WEDGED_AFTER='' GITHUB_STEP_SUMMARY="$1/summary.md" \
|
||||
bash "$1/.github/scripts/e2e-report-shape.sh" 37 "$1/gradle.log" \
|
||||
"$1/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt"
|
||||
}
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# The fixture still carries every shape.
|
||||
#
|
||||
# Three of the checks below are covered twice over -- deleting a real annotation moves the count
|
||||
# and fails a case further down. The two decoys are not: drop the KDoc mention and the count
|
||||
# stays 3, so the precision this whole ticket is about would stop being tested and nothing would
|
||||
# say so. That asymmetry is why the shapes are asserted by name rather than only by their effect
|
||||
# on the total.
|
||||
# ---------------------------------------------------------------------------
|
||||
fixture_text="$(cat -- "$FIXTURE")"
|
||||
assert_contains "fixture: the import" "$fixture_text" 'import org.libremediaconverter.FailsOnEmulatorApi37'
|
||||
assert_contains "fixture: annotation own line" "$fixture_text" '
|
||||
@FailsOnEmulatorApi37
|
||||
@Test'
|
||||
assert_contains "fixture: annotation with @Test on one line" "$fixture_text" '@FailsOnEmulatorApi37 @Test'
|
||||
assert_contains "fixture: annotation nested and indented" "$fixture_text" '
|
||||
@FailsOnEmulatorApi37'
|
||||
assert_contains "fixture: KDoc mention (this is #120)" "$fixture_text" "* Deliberately not \`@FailsOnEmulatorApi37\`"
|
||||
assert_contains "fixture: commented-out annotation" "$fixture_text" '// @FailsOnEmulatorApi37'
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# 1. The fixture's three real annotations against a baseline of 3: no deviation.
|
||||
#
|
||||
# This one case fails under both wrong matchers -- the old one counts 5, own-line-only counts 2 --
|
||||
# which is why it is first.
|
||||
# ---------------------------------------------------------------------------
|
||||
root="$(make_root "$FIXTURE_DIR" 3)"
|
||||
out="$(run_report "$root")"
|
||||
assert_contains "3 real markers, baseline 3: reports a match" "$out" ' baseline: matches (3 expected, 3 failed)'
|
||||
assert_absent "3 real markers, baseline 3: says nothing about the tree" "$out" 'the tree carries'
|
||||
assert_contains "3 real markers, baseline 3: summary agrees" \
|
||||
"$(cat -- "$root/summary.md")" '**Matches the committed baseline of 3**'
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# 2. A fourth REAL annotation. The count has to move and the deviation has to fire.
|
||||
#
|
||||
# The important half of #120: precision was the bug, but a matcher that stopped noticing a new
|
||||
# marker would have been a worse one, silently.
|
||||
# ---------------------------------------------------------------------------
|
||||
plus_one="$(mktemp -d "$TMP/plusone.XXXXXX")"
|
||||
cp -- "$FIXTURE" "$plus_one/"
|
||||
cat > "$plus_one/FourthMarker.kt" <<'EOF'
|
||||
package org.libremediaconverter.fixture
|
||||
|
||||
class FourthMarker {
|
||||
@FailsOnEmulatorApi37
|
||||
@Test
|
||||
fun addedToday() = Unit
|
||||
}
|
||||
EOF
|
||||
root="$(make_root "$plus_one" 3)"
|
||||
out="$(run_report "$root")"
|
||||
assert_contains "a 4th real marker: the deviation fires, and counts 4" "$out" \
|
||||
" baseline DEVIATION: the tree carries 4 tests marked \`@FailsOnEmulatorApi37\` but the baseline says 3 — update FAILS_ON_EMULATOR_API37_BASELINE"
|
||||
assert_contains "a 4th real marker: the summary carries it too" "$(cat -- "$root/summary.md")" \
|
||||
"- the tree carries 4 tests marked \`@FailsOnEmulatorApi37\` but the baseline says 3"
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# 3. Delete the same-line annotation and the count must drop to 2.
|
||||
#
|
||||
# This is the trap, pinned down. `@FailsOnEmulatorApi37 @Test` on one line is what separates the
|
||||
# shipped matcher from `^[[:space:]]*@NAME[[:space:]]*$`, and without this case the fixture entry
|
||||
# guarding it could be deleted as decoration -- case 1 would then pass under the wrong matcher.
|
||||
# Here the same-line entry is worth exactly one, and it is asserted to be.
|
||||
# ---------------------------------------------------------------------------
|
||||
minus_same_line="$(mktemp -d "$TMP/minus.XXXXXX")"
|
||||
sed -e '/@FailsOnEmulatorApi37 @Test/d' -- "$FIXTURE" > "$minus_same_line/MarkerShapes.kt"
|
||||
root="$(make_root "$minus_same_line" 3)"
|
||||
out="$(run_report "$root")"
|
||||
assert_contains "same-line annotation removed: counts 2, so it was worth 1" "$out" \
|
||||
" baseline DEVIATION: the tree carries 2 tests marked \`@FailsOnEmulatorApi37\` but the baseline says 3 — update FAILS_ON_EMULATOR_API37_BASELINE"
|
||||
|
||||
echo
|
||||
if [ "$failures" -eq 0 ]; then
|
||||
echo "e2e-report-shape-test.sh: all checks passed"
|
||||
exit 0
|
||||
fi
|
||||
echo "e2e-report-shape-test.sh: $failures check(s) failed"
|
||||
exit 1
|
||||
Executable
+353
@@ -0,0 +1,353 @@
|
||||
#!/usr/bin/env bash
|
||||
#
|
||||
# Reports the SHAPE of an instrumented run -- how many tests were expected, how many
|
||||
# reported, how many failed, and whether the run completed at all -- to the step log and to
|
||||
# the job summary. In advisory mode it also compares that shape against a committed baseline
|
||||
# and says plainly whether it matches.
|
||||
#
|
||||
# WHY THIS EXISTS (#83): the advisory API 37 leg is red on every PR by design, so a NEW failure
|
||||
# joining the known ones is invisible -- nothing in a red X distinguishes "the known ones" from
|
||||
# "the known ones plus yours". CLAUDE.md tells everyone not to read that job's red as their
|
||||
# change breaking something, which is correct, and which also means nobody looks.
|
||||
#
|
||||
# WHY NOT A BARE FAILURE COUNT, measured rather than assumed. On this image the run is usually
|
||||
# truncated: `Test run failed to complete. Expected 3 tests, received 2.` with
|
||||
# `INSTRUMENTATION_ABORTED: System has crashed.` A count taken from a truncated run misleads in
|
||||
# both directions -- a fourth marked test can still yield the same number if the abort lands
|
||||
# earlier, and the known set getting worse can LOWER it. So all four fields are recorded, and
|
||||
# the one saying the run was truncated is recorded with them.
|
||||
#
|
||||
# WHY IT IS A SEPARATE SCRIPT rather than a function inside e2e-run.sh: it is a pure seam. It
|
||||
# reads a captured log plus the test XML and writes a report, so it can be run against a REAL
|
||||
# log saved from a REAL CI run -- which is how the baseline comparison was shown to fire
|
||||
# without waiting on an emulator. `git ls-files '*.sh'` also picks it up for shellcheck for
|
||||
# free.
|
||||
#
|
||||
# THIS SCRIPT NEVER FAILS A RUN. It is a diagnostic, and e2e-run.sh's header explains why that
|
||||
# rule is absolute here. Every field defaults to `unknown` and every comparison is guarded,
|
||||
# because an unset variable under `set -u`, or a `[ "" -eq 3 ]`, is exactly how a diagnostic
|
||||
# becomes the thing that turns a leg red. It exits 0 unconditionally.
|
||||
#
|
||||
# Usage:
|
||||
# e2e-report-shape.sh <label> <gradle-log> [<baseline-file>]
|
||||
# E2E_WEDGED_AFTER=<seconds> the wrapper timeout killed gradle after that many seconds
|
||||
#
|
||||
# With a third argument the run is compared against the baseline in that file (advisory mode)
|
||||
# and a `::notice::` is emitted per deviation. NEVER `::error::`: the advisory job is
|
||||
# `continue-on-error: true` and stays that way, and an error annotation would be a new way for
|
||||
# a diagnostic to change a conclusion.
|
||||
#
|
||||
# WHY THE WEDGE ARRIVES AS AN ENV VAR (#118) rather than being read out of the log like every
|
||||
# other field: there is nothing in the log to read. A wedge is gradle never returning, so gradle
|
||||
# never printed a verdict, never printed a truncation line, and never aborted instrumentation --
|
||||
# the log of a wedged leg is the log of a run that simply stops. Measured on job 98035980326:
|
||||
# `expected: 59`, `received: 59`, `completed cleanly: yes`, six seconds before the wedge warning,
|
||||
# for a leg that the timeout had killed 22 minutes in. Only e2e-run.sh knows, because only it
|
||||
# saw `timeout` exit 124, so it says so. Guessing it from a log that ends abruptly would call
|
||||
# every cancelled run a wedge.
|
||||
#
|
||||
# It is read as a STRING and only ever interpolated into one. `[ -n ... ]`, never `-gt`: it
|
||||
# crosses a process boundary from a shell that deliberately sets it EMPTY on every non-wedge
|
||||
# path, and an arithmetic test on an empty string is the header's rule four paragraphs up.
|
||||
set -uo pipefail
|
||||
|
||||
LABEL="${1:-unknown}"
|
||||
LOG="${2:-}"
|
||||
BASELINE_FILE="${3:-}"
|
||||
WEDGED_AFTER="${E2E_WEDGED_AFTER:-}"
|
||||
|
||||
SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)"
|
||||
REPO_ROOT="$(cd -- "$SCRIPT_DIR/../.." && pwd)"
|
||||
XML_DIR="$REPO_ROOT/app/build/outputs/androidTest-results/connected/debug"
|
||||
|
||||
# Gradle colours its output even when it is piped, so `FAILED` arrives wrapped in escape codes.
|
||||
# The numeric lines parsed below are not coloured, but stripping is cheap insurance against a
|
||||
# pattern that would otherwise silently match nothing.
|
||||
ESC="$(printf '\033')"
|
||||
scan() { [ -s "$LOG" ] && sed -e "s/${ESC}\[[0-9;]*[a-zA-Z]//g" -- "$LOG"; }
|
||||
first_number() { grep -oE '[0-9]+' | head -1; }
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Source 1: the runner's own output. This is the ONLY place a truncation is visible. The test
|
||||
# XML read below is written even for an aborted run and says nothing whatever about the abort
|
||||
# -- measured on run 32865281555, where the XML reports a tidy tests="3" failures="3" for a run
|
||||
# the runner had just described as truncated. That is the reason this parses stdout at all.
|
||||
# ---------------------------------------------------------------------------
|
||||
starting_line="$(scan | grep -aoE 'Starting [0-9]+ tests on .*' | tail -1)"
|
||||
abort_line="$(scan | grep -aoE 'Test run failed to complete\. Expected [0-9]+ tests, received [0-9]+\.' | tail -1)"
|
||||
aborted_hits="$(scan | grep -ac 'INSTRUMENTATION_ABORTED' || true)"
|
||||
failure_line="$(scan | grep -aoE 'There was [0-9]+ failure\(s\)\.' | tail -1)"
|
||||
failed_names="$(scan | grep -aoE 'Execute [A-Za-z0-9_.$]+: FAILED' | sed -e 's/^Execute //' -e 's/: FAILED$//' | sort -u)"
|
||||
|
||||
expected="$(printf '%s' "$starting_line" | first_number)"
|
||||
expected_src="\`$starting_line\`"
|
||||
abort_expected="$(printf '%s' "$abort_line" | grep -oE 'Expected [0-9]+' | first_number)"
|
||||
abort_received="$(printf '%s' "$abort_line" | grep -oE 'received [0-9]+' | first_number)"
|
||||
log_failed="$(printf '%s' "$failure_line" | first_number)"
|
||||
|
||||
# `Starting N tests` is missing when the framework restarted under the run and Gradle never got
|
||||
# a test list. The truncation line still carries the number it was told to expect.
|
||||
if [ -z "$expected" ] && [ -n "$abort_expected" ]; then
|
||||
expected="$abort_expected"
|
||||
expected_src="\`$abort_line\`"
|
||||
fi
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Source 2: the JUnit XML. Measured on both a truncated advisory run and a green gating leg:
|
||||
# `<testsuites tests="N" failures="M">` is present in both, and aggregates every suite. It is
|
||||
# the authority on how many results landed and how many were failures. It is NOT an authority
|
||||
# on whether the run finished, which is what source 1 is for.
|
||||
# ---------------------------------------------------------------------------
|
||||
#
|
||||
# Read ONLY when the runner said a test run happened. `app/build` survives between runs on a
|
||||
# developer machine -- tools/local-emulator/run-e2e.sh drives several API levels against one
|
||||
# checkout -- so a leg that never got as far as starting tests would otherwise be reported from
|
||||
# the previous leg's XML, which is a wrong answer rather than a missing one.
|
||||
xml_head=""
|
||||
xml_count=0
|
||||
if [ -n "$starting_line$abort_line" ] && [ -d "$XML_DIR" ]; then
|
||||
while IFS= read -r f; do
|
||||
xml_count=$((xml_count + 1))
|
||||
[ -z "$xml_head" ] && xml_head="$(grep -ao '<testsuites[^>]*>' "$f" | head -1)"
|
||||
done < <(find "$XML_DIR" -maxdepth 1 -name 'TEST-*.xml' -print 2> /dev/null | sort)
|
||||
fi
|
||||
xml_tests="$(printf '%s' "$xml_head" | grep -oE ' tests="[0-9]+"' | first_number)"
|
||||
xml_failed="$(printf '%s' "$xml_head" | grep -oE ' failures="[0-9]+"' | first_number)"
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Derive the four fields, each with where its number came from. Everything stays a string, so a
|
||||
# missing source reads `unknown` rather than becoming 0 -- a report claiming 0 tests when it
|
||||
# merely could not see them would announce a deviation on every cancelled run.
|
||||
# ---------------------------------------------------------------------------
|
||||
received="unknown"
|
||||
received_src="no source"
|
||||
if [ -n "$xml_tests" ]; then
|
||||
received="$xml_tests"
|
||||
received_src="test XML \`<testsuites tests=\"$xml_tests\">\`"
|
||||
elif [ -n "$abort_received" ]; then
|
||||
received="$abort_received"
|
||||
received_src="\`$abort_line\`"
|
||||
elif [ -n "$expected" ] && [ -z "$abort_line" ]; then
|
||||
received="$expected"
|
||||
received_src="the run was not truncated, so every expected test reported"
|
||||
# ... unless it was killed, in which case "not truncated" is only "gradle never got as far as
|
||||
# saying so". This is the branch the wedged leg in #118 took -- with no XML written yet, the
|
||||
# number is what the runner was TOLD to run, and the source line said the opposite in the same
|
||||
# table that called the leg clean. The number is deliberately left alone: it is still the best
|
||||
# available answer, and only the claim about where it came from was wrong.
|
||||
[ -n "$WEDGED_AFTER" ] \
|
||||
&& received_src="no test XML was written and gradle never printed a truncation line — but the leg was killed mid-run, so this is what it was told to run, not what reported"
|
||||
fi
|
||||
|
||||
failed="unknown"
|
||||
failed_src="no source"
|
||||
if [ -n "$xml_failed" ]; then
|
||||
failed="$xml_failed"
|
||||
failed_src="test XML \`<testsuites failures=\"$xml_failed\">\`"
|
||||
elif [ -n "$log_failed" ]; then
|
||||
failed="$log_failed"
|
||||
failed_src="\`$failure_line\`"
|
||||
fi
|
||||
|
||||
if [ -z "$expected" ]; then
|
||||
expected="unknown"
|
||||
expected_src="no \`Starting N tests\` line"
|
||||
fi
|
||||
|
||||
# A run whose start nobody can see is not a run of zero tests. Cancellation (this workflow sets
|
||||
# cancel-in-progress) and the `Starting 0 tests` shape a framework restart produces both land
|
||||
# here, and both have to say so rather than compare a number that does not exist.
|
||||
no_run="none"
|
||||
if [ "$expected" = "unknown" ] && [ "$received" = "unknown" ]; then
|
||||
no_run="nothing"
|
||||
elif [ "$expected" = "0" ]; then
|
||||
no_run="zero"
|
||||
fi
|
||||
|
||||
if [ -n "$abort_line" ]; then
|
||||
completed="**no**"
|
||||
completed_src="\`$abort_line\` with \`INSTRUMENTATION_ABORTED\`"
|
||||
elif [ "${aborted_hits:-0}" -gt 0 ]; then
|
||||
completed="**no**"
|
||||
completed_src="\`INSTRUMENTATION_ABORTED\` in the runner output"
|
||||
elif [ "$no_run" = "nothing" ]; then
|
||||
# "cleanly" would be a lie about a run that left no evidence it happened.
|
||||
completed="unknown"
|
||||
completed_src="no runner output to read"
|
||||
elif [ -n "$WEDGED_AFTER" ]; then
|
||||
# The wedge is checked LAST of the four, so it only ever overrides the `yes`. The two "no"s
|
||||
# above are already right and name the abort, which the wedge row does not; `unknown` is
|
||||
# already right too. A wedge on top of an abort is both facts, and both get printed.
|
||||
completed="**no**"
|
||||
completed_src="the wrapper timeout killed gradle after ${WEDGED_AFTER}s — instrumentation itself was never aborted, which is why nothing in the log says so"
|
||||
else
|
||||
completed="yes"
|
||||
completed_src="no truncation line and no \`INSTRUMENTATION_ABORTED\`"
|
||||
fi
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Advisory mode: compare against the committed baseline.
|
||||
#
|
||||
# ONE number covers both compared fields, and that is deliberate rather than a shortcut. The
|
||||
# marker means "cannot pass on this image", so the number of tests carrying it is both how many
|
||||
# the advisory leg should run and how many should fail. A smaller `failed` means one now passes
|
||||
# -- which is the trigger to delete the annotation, written down in FailsOnEmulatorApi37.kt.
|
||||
# ---------------------------------------------------------------------------
|
||||
#
|
||||
# `advisory` and `baseline` are two variables on purpose. "A comparison was asked for" and "a
|
||||
# number was found to compare against" are different facts, and collapsing them is how this
|
||||
# report would go quietly back to being the thing #83 filed: the `sed` below is anchored, so
|
||||
# indenting the const into an object -- or renaming it, or moving it to another file -- empties
|
||||
# `baseline`, and a single flag would take the whole comparison down with it while the table
|
||||
# kept printing. An unreadable baseline is itself a deviation, and is announced as one.
|
||||
advisory="no"
|
||||
baseline=""
|
||||
marked=""
|
||||
deviations=()
|
||||
if [ -n "$BASELINE_FILE" ]; then
|
||||
advisory="yes"
|
||||
[ -f "$BASELINE_FILE" ] \
|
||||
&& baseline="$(sed -nE 's/^const val FAILS_ON_EMULATOR_API37_BASELINE = ([0-9]+).*/\1/p' "$BASELINE_FILE" | head -1)"
|
||||
# What the tree actually carries. Reported next to the baseline so a stale baseline shows up
|
||||
# here rather than only once the emulator disagrees with it.
|
||||
#
|
||||
# ANCHORED AT LINE START, AND WHITESPACE-OR-END-OF-LINE AFTER THE NAME (#120). The #81 check
|
||||
# this replaces matched the string anywhere on any line, so #113's KDoc reading `Deliberately
|
||||
# not @FailsOnEmulatorApi37` counted as a fourth marked test and the report announced a
|
||||
# deviation on every PR. That is worse than a wrong number: #83 built this so a new failure
|
||||
# could not be invisible, and a notice that is wrong every time teaches everyone to skim past
|
||||
# deviation notices.
|
||||
#
|
||||
# THE OBVIOUS REPAIR IS A TRAP, and the reason for the second half of the pattern.
|
||||
# `^[[:space:]]*@NAME[[:space:]]*$` -- "the annotation on a line of its own" -- also stops
|
||||
# counting `@FailsOnEmulatorApi37 @Test`, which is legal Kotlin, and UNDERcounting is the
|
||||
# dangerous direction: it hides a genuine new marker, which is the one thing this exists to
|
||||
# catch. Measured against `testdata/marker-shapes`, a fixture carrying every shape at once:
|
||||
# the old matcher says 5, own-line-only says 2, this one says 3. On the real tree, 4 / 3 / 3.
|
||||
# e2e-report-shape-test.sh runs that fixture through this whole script.
|
||||
#
|
||||
# `^[[:space:]]*@` cannot match an `import` line, so the old `grep -v import` goes with it
|
||||
# rather than staying to imply a filter is still doing work.
|
||||
#
|
||||
# This is a regex over source text and not a parser. An annotation inside a multi-line string,
|
||||
# or inside a `/* */` block that opened mid-line, would still be counted. Neither exists here;
|
||||
# if one ever does, this check wants a different tool rather than a longer regex.
|
||||
if [ -d "$REPO_ROOT/app/src/androidTest" ]; then
|
||||
marked="$(grep -rhcE '^[[:space:]]*@FailsOnEmulatorApi37([[:space:]]|$)' \
|
||||
"$REPO_ROOT/app/src/androidTest" --include='*.kt' \
|
||||
| awk '{ total += $1 } END { print total + 0 }' || true)"
|
||||
fi
|
||||
fi
|
||||
|
||||
if [ "$advisory" = "yes" ] && [ -z "$baseline" ]; then
|
||||
deviations+=("the committed baseline could not be read from \`$(basename -- "$BASELINE_FILE")\` — has \`FAILS_ON_EMULATOR_API37_BASELINE\` been renamed, indented into a class, or moved? Nothing was compared")
|
||||
fi
|
||||
|
||||
if [ -n "$baseline" ]; then
|
||||
if [ "$no_run" = "nothing" ]; then
|
||||
deviations+=("no test run observed — the runner never reported starting one, where the baseline expects $baseline tests carrying \`@FailsOnEmulatorApi37\`")
|
||||
elif [ "$no_run" = "zero" ]; then
|
||||
deviations+=("the runner started 0 tests, where the baseline expects $baseline — on this image that is the framework having restarted under the run, not an empty test list")
|
||||
else
|
||||
if [ "$expected" != "unknown" ] && [ "$expected" != "$baseline" ]; then
|
||||
deviations+=("the runner started $expected tests, the baseline is $baseline")
|
||||
fi
|
||||
if [ "$failed" != "unknown" ] && [ "$failed" != "$baseline" ]; then
|
||||
deviations+=("$failed tests failed, the baseline is $baseline — every test carrying the marker is expected to fail on this image, so fewer means one now passes and more means a new one joined")
|
||||
fi
|
||||
fi
|
||||
if [ -n "$marked" ] && [ "$marked" != "$baseline" ]; then
|
||||
deviations+=("the tree carries $marked tests marked \`@FailsOnEmulatorApi37\` but the baseline says $baseline — update FAILS_ON_EMULATOR_API37_BASELINE")
|
||||
fi
|
||||
fi
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Emit. Step log first, so the common case needs neither the summary page nor an artifact.
|
||||
# ---------------------------------------------------------------------------
|
||||
echo "----- RUN SHAPE (api${LABEL}) -----"
|
||||
echo " expected: $expected"
|
||||
echo " received: $received"
|
||||
echo " failed: $failed"
|
||||
# Above `completed cleanly`, because it is the line that says what happened to the leg and the
|
||||
# other one only qualifies it. A reader who stops after three rows still sees it.
|
||||
if [ -n "$WEDGED_AFTER" ]; then
|
||||
echo " wedged: yes -- gradle was killed after ${WEDGED_AFTER}s and never returned"
|
||||
fi
|
||||
echo " completed cleanly: ${completed//\*/}"
|
||||
if [ -n "$abort_received" ]; then
|
||||
echo " received before the abort: $abort_received"
|
||||
fi
|
||||
if [ -n "$failed_names" ]; then
|
||||
echo " failed tests:"
|
||||
printf '%s\n' "$failed_names" | sed -e 's/^/ /'
|
||||
fi
|
||||
if [ "$advisory" = "yes" ]; then
|
||||
if [ "${#deviations[@]}" -eq 0 ]; then
|
||||
echo " baseline: matches ($baseline expected, $baseline failed)"
|
||||
else
|
||||
printf ' baseline DEVIATION: %s\n' "${deviations[@]}"
|
||||
fi
|
||||
fi
|
||||
|
||||
# A notice, never an error. See the header.
|
||||
if [ "${#deviations[@]}" -gt 0 ]; then
|
||||
for d in "${deviations[@]}"; do
|
||||
echo "::notice::E2E api${LABEL}: $d"
|
||||
done
|
||||
fi
|
||||
|
||||
if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then
|
||||
{
|
||||
echo "### E2E api${LABEL} — run shape"
|
||||
echo
|
||||
echo "| field | value | where it came from |"
|
||||
echo "| --- | --- | --- |"
|
||||
echo "| expected | $expected | $expected_src |"
|
||||
echo "| received | $received | $received_src |"
|
||||
echo "| failed | $failed | $failed_src |"
|
||||
if [ -n "$WEDGED_AFTER" ]; then
|
||||
echo "| wedged | **yes** | \`timeout\` fired after ${WEDGED_AFTER}s and killed gradle (exit 124), which is what e2e-run.sh then captured the wedge diagnostics for |"
|
||||
fi
|
||||
echo "| completed cleanly | $completed | $completed_src |"
|
||||
if [ -n "$abort_received" ]; then
|
||||
echo "| received before the abort | $abort_received | the same line — the XML above counts the truncated test as a failure, this number does not |"
|
||||
fi
|
||||
echo
|
||||
if [ -n "$failed_names" ]; then
|
||||
echo "Failed:"
|
||||
echo
|
||||
printf '%s\n' "$failed_names" | sed -e 's/^/- `/' -e 's/$/`/'
|
||||
echo
|
||||
fi
|
||||
if [ "$xml_count" -gt 1 ]; then
|
||||
echo "> $xml_count test XML files were present; the counts above come from the first."
|
||||
echo
|
||||
fi
|
||||
if [ "$advisory" = "yes" ]; then
|
||||
if [ "${#deviations[@]}" -eq 0 ]; then
|
||||
echo "**Matches the committed baseline of $baseline** — $baseline tests carry \`@FailsOnEmulatorApi37\` and all $baseline failed, which is what this job is for."
|
||||
elif [ -z "$baseline" ]; then
|
||||
echo "**The committed baseline could not be read, so nothing was compared.** Announced as a notice, not an error: this job is advisory and its conclusion is unchanged by anything here."
|
||||
echo
|
||||
printf -- '- %s\n' "${deviations[@]}"
|
||||
else
|
||||
echo "**DEVIATION from the committed baseline of $baseline.** Announced as a notice, not an error: this job is advisory and its conclusion is unchanged by anything here."
|
||||
echo
|
||||
printf -- '- %s\n' "${deviations[@]}"
|
||||
fi
|
||||
echo
|
||||
echo "<sub>The baseline lives beside the marker, in \`FailsOnEmulatorApi37.kt\`. \`completed cleanly\` is recorded rather than compared: the truncation is intermittent — of eight advisory runs read on 2026-08-25, seven aborted and one did not — so comparing it would announce a deviation on a run that is fine.</sub>"
|
||||
else
|
||||
echo "<sub>No baseline comparison: that is the advisory API 37 leg only. The shape is recorded here anyway because a truncated run reports fewer results than it ran, which is what issue #108 looks like on a gating leg.</sub>"
|
||||
fi
|
||||
echo
|
||||
} >> "$GITHUB_STEP_SUMMARY"
|
||||
# The summary page is the deliverable -- "readable without opening a log" is what #83 asked
|
||||
# for -- and GitHub exposes no API for reading a job summary back, so a write that silently
|
||||
# did not happen would be invisible. This line is in the step log, which can be read.
|
||||
echo " (the table above is also on the job summary page)"
|
||||
else
|
||||
echo " (GITHUB_STEP_SUMMARY is unset -- step log only)"
|
||||
fi
|
||||
|
||||
exit 0
|
||||
@@ -34,6 +34,13 @@ TMP="${RUNNER_TEMP:-/tmp}"
|
||||
LOGCAT_LOG="$TMP/logcat-api${LABEL}.txt"
|
||||
DIAG_LOG="$TMP/diagnostics-api${LABEL}.txt"
|
||||
WEDGE_LOG="$TMP/wedge-diagnostics-api${LABEL}.txt"
|
||||
# Gradle's own output, captured to a file as well as the step log, because the run-shape report
|
||||
# below has to parse it. Uploaded with the diagnostics, so a report that reads wrong can be
|
||||
# checked against what it read.
|
||||
GRADLE_LOG="$TMP/gradle-api${LABEL}.txt"
|
||||
|
||||
SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)"
|
||||
REPO_ROOT="$(cd -- "$SCRIPT_DIR/../.." && pwd)"
|
||||
|
||||
# ~5 min is a healthy leg (measured across API 33-36), and this wraps only the gradle client,
|
||||
# a subset of that. 20 min is generous enough never to trip on a slow-but-working run, and far
|
||||
@@ -214,18 +221,60 @@ status=0
|
||||
# script rather than forking it: that runs several API levels back to back against one checkout
|
||||
# and passes `--rerun`, so a level cannot be skipped as up-to-date and report the previous
|
||||
# level's results as its own. CI gets a fresh runner per level and does not need it.
|
||||
#
|
||||
# `2>&1 | tee`, and the `2>&1` is the load-bearing half. The step log merges both streams, so
|
||||
# reading one cannot tell you which stream a line came from -- and the single line the report
|
||||
# below needs most, `Test run failed to complete. ... INSTRUMENTATION_ABORTED`, is not on
|
||||
# stdout. Capturing stdout alone would leave the report saying "completed cleanly: yes" forever,
|
||||
# which is precisely the comparison that cannot fire. pipefail is already set and tee exits 0,
|
||||
# so the pipeline's status is still gradle's -- including the 124 that means the wrapper fired.
|
||||
#
|
||||
# `tee` and not `tee -a`, unlike the logcat above: CI gets a fresh runner per leg, but
|
||||
# tools/local-emulator/run-e2e.sh reuses one machine, and an appended log would have the report
|
||||
# reading the PREVIOUS run of the same API level. The console goes plain rather than showing
|
||||
# gradle's live progress bar, which is what it already did in CI.
|
||||
# shellcheck disable=SC2086
|
||||
timeout -k 30s "$WEDGE_TIMEOUT" \
|
||||
./gradlew :app:connectedDebugAndroidTest -PabiFilters=x86_64 --stacktrace \
|
||||
${E2E_EXTRA_GRADLE_ARGS:-} || status=$?
|
||||
${E2E_EXTRA_GRADLE_ARGS:-} 2>&1 | tee "$GRADLE_LOG" || status=$?
|
||||
echo "::endgroup::"
|
||||
|
||||
# Whether the wrapper timeout fired, decided ONCE. 124 is `timeout` saying it killed the
|
||||
# command, and two places downstream need that fact: capture_wedge below, and the report, which
|
||||
# otherwise calls a killed leg `completed cleanly: yes` (#118). Deriving it twice is how those
|
||||
# two would drift apart -- the report would keep printing after someone changed what a wedge
|
||||
# means here. It stays a string: empty on every other path, so those legs pass an empty
|
||||
# E2E_WEDGED_AFTER and the report behaves exactly as before.
|
||||
wedged=""
|
||||
[ "$status" -eq 124 ] && wedged="$WEDGE_TIMEOUT"
|
||||
|
||||
# The run-shape report: expected/received/failed and whether the run finished, every time,
|
||||
# green or red. It never changes `status` -- it is a diagnostic, and the header's rule about
|
||||
# diagnostics applies to it as much as to every probe below.
|
||||
#
|
||||
# E2E_WEDGED_AFTER is the wedge, told to the report rather than left for it to infer. It cannot
|
||||
# be inferred: a wedge is gradle never returning, so gradle printed no verdict at all, and the
|
||||
# log the report reads looks like a run that simply stopped. Only this script knows the
|
||||
# difference, because only this script saw the exit status.
|
||||
#
|
||||
# The baseline argument, and only it, turns on the comparison, and only the advisory API 37 job
|
||||
# passes E2E_ADVISORY=1. Comparing on the gating legs would announce a deviation on all five of
|
||||
# them every run, since they run the whole suite rather than the marked three. They still get
|
||||
# the report: a truncated run reporting fewer results than it ran is what #108 looks like, and
|
||||
# `completed cleanly` is the field that shows it.
|
||||
if [ "${E2E_ADVISORY:-}" = "1" ]; then
|
||||
E2E_WEDGED_AFTER="$wedged" bash "$SCRIPT_DIR/e2e-report-shape.sh" "$LABEL" "$GRADLE_LOG" \
|
||||
"$REPO_ROOT/app/src/androidTest/java/org/libremediaconverter/FailsOnEmulatorApi37.kt" || true
|
||||
else
|
||||
E2E_WEDGED_AFTER="$wedged" bash "$SCRIPT_DIR/e2e-report-shape.sh" "$LABEL" "$GRADLE_LOG" || true
|
||||
fi
|
||||
|
||||
if [ "$status" -eq 0 ]; then
|
||||
kill "$LOGCAT_PID" 2>/dev/null || true
|
||||
exit 0
|
||||
fi
|
||||
|
||||
if [ "$status" -eq 124 ]; then
|
||||
if [ -n "$wedged" ]; then
|
||||
capture_wedge "api${LABEL}"
|
||||
else
|
||||
echo "::error::E2E api${LABEL} failed (exit $status)"
|
||||
|
||||
@@ -0,0 +1,54 @@
|
||||
// NOT A TEST, AND NEVER COMPILED. This is fixture data for e2e-report-shape-test.sh, which
|
||||
// copies it into a throwaway repo root and runs the real report script against that. It lives
|
||||
// under .github/ deliberately: Gradle only compiles app/src/**, ktlint and detekt are applied to
|
||||
// :app only, and the report's own count reads app/src/androidTest -- so nothing here can reach
|
||||
// the build, the linters, or the number the advisory job compares against. Verified by running
|
||||
// the report against the real repo root with this file committed: still 3.
|
||||
//
|
||||
// It carries every shape the counter has to tell apart, in one file, because the bug in #120 was
|
||||
// exactly that two of them look alike to a substring match. Three count and three must not:
|
||||
//
|
||||
// COUNTS the annotation on its own line
|
||||
// COUNTS the annotation sharing a line with @Test -- legal Kotlin, and the case the
|
||||
// obvious "own line only" repair silently drops
|
||||
// COUNTS the annotation indented inside a nested class
|
||||
// must NOT a KDoc mentioning it -- this is #120 itself, copied from Media3EngineTest
|
||||
// must NOT a commented-out annotation
|
||||
// must NOT the import
|
||||
//
|
||||
// Three count. That is what the synthetic baseline in the test is set to, so the fixture and the
|
||||
// baseline agree exactly the way the real tree and FAILS_ON_EMULATOR_API37_BASELINE do.
|
||||
//
|
||||
// The `@Test` here is spelled the way a real test spells it so the fixture reads like source
|
||||
// rather than like a regex exercise. Nothing runs it.
|
||||
|
||||
package org.libremediaconverter.fixture
|
||||
|
||||
import org.junit.Test
|
||||
import org.libremediaconverter.FailsOnEmulatorApi37
|
||||
|
||||
class MarkerShapes {
|
||||
@FailsOnEmulatorApi37
|
||||
@Test
|
||||
fun ownLine() = Unit
|
||||
|
||||
@FailsOnEmulatorApi37 @Test
|
||||
fun sameLineAsTest() = Unit
|
||||
|
||||
/**
|
||||
* Deliberately not `@FailsOnEmulatorApi37`: nothing here decodes or encodes, so no emulator
|
||||
* codec is involved and the API 37 image has no quarrel with it.
|
||||
*/
|
||||
@Test
|
||||
fun mentionedInKdoc() = Unit
|
||||
|
||||
// @FailsOnEmulatorApi37 -- taken off on 2026-01-01, kept as a note rather than deleted
|
||||
@Test
|
||||
fun commentedOut() = Unit
|
||||
|
||||
class Nested {
|
||||
@FailsOnEmulatorApi37
|
||||
@Test
|
||||
fun indentedDeeper() = Unit
|
||||
}
|
||||
}
|
||||
@@ -13,6 +13,17 @@ on:
|
||||
# reference amounts to running whatever that repository contains tomorrow. This matters
|
||||
# more here than on pull requests: these jobs sign nothing today, but they do publish
|
||||
# the artifacts people install.
|
||||
# Declared here rather than inherited, for the reason status_check.yml gives for its own
|
||||
# block: the token's reach should be readable in the file that uses it, and a repository
|
||||
# default that widens later should not silently widen these jobs with it. The repository
|
||||
# default is `read` today, so this changes nothing about what runs -- it fixes what a
|
||||
# reader can know without leaving the file, and it is what CodeQL alert #1 asked for.
|
||||
#
|
||||
# The `release` job below overrides this with `contents: write`, which is how job-level
|
||||
# permissions work: this is a default, not a ceiling.
|
||||
permissions:
|
||||
contents: read
|
||||
|
||||
env:
|
||||
GRADLE_CACHE_PATHS: |
|
||||
~/.gradle/caches
|
||||
|
||||
@@ -280,8 +280,13 @@ jobs:
|
||||
# docs/api-37-emulator-crash.md has the per-method measurements, and the
|
||||
# correction that produced them.
|
||||
#
|
||||
# api-level must be "37.0". A bare 37 is not an SDK package and fails
|
||||
# during setup, which cost a run to discover.
|
||||
# api-level must be a POINT release. A bare 37 is not an SDK package and
|
||||
# fails during setup, which cost a run to discover. `37.0` is the choice
|
||||
# here rather than the only option: `37.1` and `37.2-beta*` exist and
|
||||
# abort the same way, and api37-debug.yml's inputs document both, with
|
||||
# the wrinkle that above 37.0 they ship only as google_apis_ps16k.
|
||||
# docs/api-37-emulator-crash.md measures 37.0 rev 6 and 37.1 rev 8 side
|
||||
# by side, so pinning 37.0 is a decision, not a constraint.
|
||||
#
|
||||
# notAnnotation removes the three tests that do not pass on this image; they
|
||||
# run in the advisory job below, off the same marker so they cannot end up
|
||||
@@ -372,6 +377,7 @@ jobs:
|
||||
path: |
|
||||
${{ runner.temp }}/logcat-api${{ matrix.label }}.txt
|
||||
${{ runner.temp }}/diagnostics-api${{ matrix.label }}.txt
|
||||
${{ runner.temp }}/gradle-api${{ matrix.label }}.txt
|
||||
if-no-files-found: warn
|
||||
|
||||
# Only exists when the wrapper timeout tripped, so `ignore` keeps healthy runs quiet
|
||||
@@ -462,6 +468,12 @@ jobs:
|
||||
# The complement of the gating row's notAnnotation, off the same marker,
|
||||
# so a test can never be excluded from both jobs or run in both.
|
||||
E2E_EXTRA_GRADLE_ARGS: "-Pandroid.testInstrumentationRunnerArguments.annotation=org.libremediaconverter.FailsOnEmulatorApi37"
|
||||
# Turns on the baseline comparison in the run-shape report, and only here. Every leg
|
||||
# prints the shape; this is the one that also says whether it matches
|
||||
# FAILS_ON_EMULATOR_API37_BASELINE, because this is the one whose test list is the
|
||||
# marker. A deviation is a `::notice::` -- this job stays continue-on-error and stays
|
||||
# out of the required contexts, so nothing the report finds can change a conclusion.
|
||||
E2E_ADVISORY: "1"
|
||||
with:
|
||||
# Every device pin below matches the gating row exactly, so a difference
|
||||
# between the two jobs is the test selection and nothing else.
|
||||
@@ -494,6 +506,7 @@ jobs:
|
||||
path: |
|
||||
${{ runner.temp }}/logcat-api${{ env.E2E_LABEL }}.txt
|
||||
${{ runner.temp }}/diagnostics-api${{ env.E2E_LABEL }}.txt
|
||||
${{ runner.temp }}/gradle-api${{ env.E2E_LABEL }}.txt
|
||||
if-no-files-found: warn
|
||||
|
||||
- uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
|
||||
|
||||
@@ -76,11 +76,11 @@ days. Read it as the current answer, and see the git history if you need the old
|
||||
`angle_indirect` and `swangle_indirect` all boot, while `auto`, `off`, `guest` and
|
||||
`swiftshader_indirect` do not. `docs/local-emulator.md` has the evidence and the per-API renderer
|
||||
table.
|
||||
- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Three** of the 59 instrumented
|
||||
- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Three** of the 60 instrumented
|
||||
tests cannot pass on that image, for two unrelated reasons: two Media3 hardware transcodes fail
|
||||
inside the emulator's own `c2.goldfish.h264.decoder`, and one SAF test takes the framework down
|
||||
when it rotates the display. All three carry `@FailsOnEmulatorApi37` and run in a separate
|
||||
`continue-on-error` job; the gating leg runs the other 56.
|
||||
`continue-on-error` job; the gating leg runs the other 57.
|
||||
|
||||
That job is still called `E2E API 37 Media3 hardware transcode (advisory)`, which no longer
|
||||
describes everything in it. The name is kept deliberately — it is not a required context and
|
||||
@@ -89,6 +89,22 @@ days. Read it as the current answer, and see the git history if you need the old
|
||||
not read a green run as evidence those three tests pass.
|
||||
`docs/api-37-emulator-crash.md` has the measurements.
|
||||
|
||||
**That instruction is also why nobody looks, so the job now reports its own shape** — expected,
|
||||
received, failed, and whether the run completed — to the job summary, and compares it against
|
||||
`FAILS_ON_EMULATOR_API37_BASELINE`, committed beside the marker. A deviation is a `::notice::`;
|
||||
the job stays advisory and its conclusion is untouched. **Add or remove a `@FailsOnEmulatorApi37`
|
||||
and that number changes in the same diff**, or the next run says so. A bare failure count would
|
||||
not have worked: the run is usually truncated by an `INSTRUMENTATION_ABORTED`, and the test XML
|
||||
is written anyway and says nothing about it — `.github/scripts/e2e-report-shape.sh` is where that
|
||||
is measured and explained.
|
||||
|
||||
Every leg prints that table, advisory or not, and **on the wedge path it also carries a `wedged:`
|
||||
row** (#118). `completed cleanly` only ever meant "instrumentation was not aborted", which stays
|
||||
true of a leg the `WEDGE_TIMEOUT` killed 22 minutes in — so without that row the table read
|
||||
`received: 59, completed cleanly: yes` for a leg that had just died. The wedge is passed to the
|
||||
report as `E2E_WEDGED_AFTER` by `e2e-run.sh`, which is the only thing that can know it: a wedge
|
||||
is gradle never returning, so the log it left says nothing about it.
|
||||
|
||||
Still true, and the reason the advisory job is not simply deleted: **API 37 needs a manual check on
|
||||
the Pixel 10 Pro XL before each release.** Those three tests are the one thing CI cannot answer
|
||||
for.
|
||||
@@ -114,10 +130,10 @@ install for code that can never run — and on API 37 the full APK does not fit
|
||||
- The `model` package is excluded from `ReturnCount` and `CyclomaticComplexMethod` only. It is the
|
||||
decision layer, where one branch is one documented user-visible outcome and the metric counts
|
||||
answers rather than complexity. Every other rule still applies there.
|
||||
- **Coverage is reported, not gated** — **69.2% of lines (1519/2194), 53.2% of branches**,
|
||||
measured 2026-08-24 with `./gradlew :app:jacocoTestReport`.
|
||||
- **Coverage is reported, not gated** — **84.9% of lines (1971/2321), 63.8% of branches**,
|
||||
measured 2026-08-26 with `./gradlew :app:jacocoTestReport`, against 454 JVM tests in 67 classes.
|
||||
|
||||
**Every figure this file carried before that date was an artifact, roughly half the real one.**
|
||||
**Every figure this file carried before 2026-08-24 was an artifact, roughly half the real one.**
|
||||
Robolectric loads classes through its own sandbox classloader with no source location, JaCoCo
|
||||
skips no-location classes by default, and nothing told it otherwise — so **not one Robolectric
|
||||
test counted**, and Robolectric is what exercises the framework edge here. The
|
||||
@@ -131,9 +147,12 @@ install for code that can never run — and on API 37 the full APK does not fit
|
||||
disproportionately Robolectric, so each one added denominator and no numerator — the measurement
|
||||
was punishing exactly the tests that were hardest to write.
|
||||
|
||||
Two things still hold. A floor needs a baseline that has settled, and this one has now moved by
|
||||
39 points in a single build change, so it has not. And **re-measure before quoting** — that
|
||||
instruction is the only reason this was caught.
|
||||
Two things still hold. A floor needs a baseline that has settled, and this one has not: it moved
|
||||
39 points in a single build change on 2026-08-24, then another 16 as the #52 test push and the
|
||||
fixes it turned up landed — 69.2% -> 84.9% line, 53.2% -> 63.8% branch — while the denominator
|
||||
grew 2194 -> 2321, because that work added production code of its own. And **re-measure before
|
||||
quoting**: this entry was written quoting 81.4%, measured four hours earlier, and was already
|
||||
three points stale by the time it was ready to merge.
|
||||
- **Testable code is not done until it is tested.** If a piece is unit testable, it gets unit
|
||||
tests before it counts as done. If it is e2e testable, it gets e2e tests. Both clauses apply —
|
||||
a change that is both needs both.
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import org.gradle.api.tasks.PathSensitivity
|
||||
import org.gradle.testing.jacoco.tasks.JacocoReport
|
||||
|
||||
plugins {
|
||||
@@ -206,6 +207,16 @@ detekt {
|
||||
// `excludes` is not optional. Without it JaCoCo walks JDK-internal classes that Robolectric has
|
||||
// no location for either, and the test JVM dies rather than reporting a number.
|
||||
tasks.withType<Test>().configureEach {
|
||||
// ReleasePermissionTest reads .github/workflows/build.yml, and Gradle cannot infer that a
|
||||
// test depends on a file outside the source set. Without this the task stays UP-TO-DATE
|
||||
// when the workflow changes, so the guard goes stale exactly when it matters. Measured:
|
||||
// deleting the release job's `contents: write` and re-running gave "BUILD SUCCESSFUL in
|
||||
// 614ms" with the test never executing; the same mutation under --rerun-tasks failed it.
|
||||
// A guard that does not re-run when its subject changes is not a guard.
|
||||
inputs.file(rootProject.file(".github/workflows/build.yml"))
|
||||
.withPropertyName("releaseWorkflow")
|
||||
.withPathSensitivity(PathSensitivity.RELATIVE)
|
||||
|
||||
extensions.configure<JacocoTaskExtension> {
|
||||
isIncludeNoLocationClasses = true
|
||||
excludes = listOf("jdk.internal.*")
|
||||
|
||||
@@ -18,7 +18,38 @@ package org.libremediaconverter
|
||||
* Removing it is the goal, and the trigger is written down: a new API 37.x system image, or an
|
||||
* ATD image for 37. Delete the annotation from the tests, and the advisory job goes empty and
|
||||
* the gating one grows by two.
|
||||
*
|
||||
* **How many tests carry it is committed below**, as [FAILS_ON_EMULATOR_API37_BASELINE], and the
|
||||
* advisory job checks the run against it. Adding or removing a marker means changing that number
|
||||
* in the same diff.
|
||||
*/
|
||||
@Retention(AnnotationRetention.RUNTIME)
|
||||
@Target(AnnotationTarget.CLASS, AnnotationTarget.FUNCTION)
|
||||
annotation class FailsOnEmulatorApi37
|
||||
|
||||
/**
|
||||
* How many tests carry [FailsOnEmulatorApi37] — the advisory API 37 job's committed baseline.
|
||||
*
|
||||
* **No Kotlin reads this, and it is not stray config.** `.github/scripts/e2e-report-shape.sh`
|
||||
* parses it out of this file by name, with a line-anchored pattern, and the advisory job compares
|
||||
* the run it just did against it: this many tests should start, and all of them should fail.
|
||||
* Deleting it, renaming it, or indenting it into a class stops the comparison — the report would
|
||||
* keep printing with nothing to compare to, so it announces that it could not read the baseline
|
||||
* rather than falling quiet. If you see that notice, this line is what it means.
|
||||
*
|
||||
* **One number, both checks, and that is what the marker means.** A test carrying it cannot pass
|
||||
* on this image, so the count is simultaneously how many the advisory leg runs and how many fail.
|
||||
* A *smaller* failure count is the interesting direction: it means one of them now passes, which
|
||||
* is the trigger the KDoc above names for deleting the annotation.
|
||||
*
|
||||
* So: adding or removing a [FailsOnEmulatorApi37] means changing this number, in this file, in
|
||||
* the same diff. The report says so on the run itself if you forget — it prints the tree's own
|
||||
* `grep` count beside this one.
|
||||
*
|
||||
* Why a baseline at all (#83): that job is `continue-on-error` and red on every PR by design, so
|
||||
* a red X cannot distinguish the known failures from the known failures plus a new one. Counting
|
||||
* failures alone does not fix it either — the run is usually truncated by an
|
||||
* `INSTRUMENTATION_ABORTED`, so the count is a number taken from a partial run. The report
|
||||
* records the truncation next to the counts for that reason.
|
||||
*/
|
||||
const val FAILS_ON_EMULATOR_API37_BASELINE = 3
|
||||
|
||||
@@ -36,8 +36,20 @@ import java.io.File
|
||||
* 1. that the hardware path is worth having a second engine for at all, and
|
||||
* 2. that x264's CRF is worth the GPL licence the app carries for it.
|
||||
*
|
||||
* Skips itself when the sample files are absent, so it is harmless in CI. Populate with:
|
||||
* adb push <file>.mp4 /sdcard/Android/data/org.libremediaconverter/files/
|
||||
* Skips itself when the sample files are absent, so it is harmless in CI — every green E2E
|
||||
* leg reports two skips, and these are they.
|
||||
*
|
||||
* The two files it looks for, by exact name:
|
||||
*
|
||||
* - [H264_SAMPLE] for [hardwareVersusSoftwareOnRealVideo]
|
||||
* - [AV1_SAMPLE] for [av1InputRoutesAccordingToDeviceDecodeSupport]
|
||||
*
|
||||
* **Where they go, and how, is on [samples] — read it before staging anything.** This used to
|
||||
* carry an `adb push` line naming the external files dir, which [samples] then explains cannot
|
||||
* work: a pushed file stays owned by the shell user and the app reads EACCES, surfacing as an
|
||||
* unparseable input rather than a permission error. The instruction and its own refutation sat
|
||||
* twelve lines apart. It is named in one place now rather than restated here, because restating
|
||||
* it is what let the two drift.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(AndroidJUnit4::class)
|
||||
|
||||
@@ -11,15 +11,26 @@ import kotlinx.coroutines.runBlocking
|
||||
import kotlinx.coroutines.withTimeout
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertNull
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.FailsOnEmulatorApi37
|
||||
import org.libremediaconverter.model.AudioCodec
|
||||
import org.libremediaconverter.model.AudioPlan
|
||||
import org.libremediaconverter.model.Container
|
||||
import org.libremediaconverter.model.ConversionRequest
|
||||
import org.libremediaconverter.model.CopyPlanner
|
||||
import org.libremediaconverter.model.InputKind
|
||||
import org.libremediaconverter.model.InputProbe
|
||||
import org.libremediaconverter.model.OutputFormat
|
||||
import org.libremediaconverter.model.OutputSpec
|
||||
import org.libremediaconverter.model.VideoCodec
|
||||
import org.libremediaconverter.model.VideoPlan
|
||||
import java.io.File
|
||||
import java.util.concurrent.CancellationException
|
||||
import java.util.concurrent.Executors
|
||||
import java.util.concurrent.TimeUnit
|
||||
|
||||
@@ -170,6 +181,63 @@ class Media3EngineTest {
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* The builders that used to throw where nothing could catch them.
|
||||
*
|
||||
* `EditedMediaItem.Builder` rejects a composition with both tracks removed —
|
||||
* checkState("Audio and video cannot both be removed") — and the engine builds it on its own
|
||||
* HandlerThread. That build sat *between* two narrow `runCatching` blocks, one around
|
||||
* `buildTransformer` and one around `start`, so the exception reached the thread's uncaught
|
||||
* handler and took the process with it while the continuation was never resumed.
|
||||
*
|
||||
* `ContainerCapabilities.validate` now refuses the spec that gets here from the picker; this
|
||||
* is the other half — the engine surviving a request that arrives without being validated.
|
||||
* Deliberately not `@FailsOnEmulatorApi37`: nothing here decodes or encodes, so no emulator
|
||||
* codec is involved. The builder refuses the input before any media is touched.
|
||||
*/
|
||||
@Test
|
||||
fun aPlanThatRemovesBothTracksFailsInsteadOfKillingTheProcess() {
|
||||
val request = ConversionRequest(
|
||||
spec = OutputSpec(Container.MP4, VideoCodec.H265, AudioCodec.NONE),
|
||||
probe = InputProbe(
|
||||
videoCodec = null,
|
||||
audioCodec = "mp3",
|
||||
hasVideo = false,
|
||||
container = Container.MP3,
|
||||
kind = InputKind.AUDIO_ONLY,
|
||||
),
|
||||
)
|
||||
// Asserted rather than assumed: ConversionRequest's default probe says hasVideo = true,
|
||||
// and with it this same spec plans to (Encode, Drop) and nothing throws at all — which
|
||||
// would make the whole test vacuous without a word of warning.
|
||||
val plan = CopyPlanner.plan(request.spec, request.probe)
|
||||
assertEquals(VideoPlan.Drop, plan.video)
|
||||
assertEquals(AudioPlan.Drop, plan.audio)
|
||||
|
||||
val failure = runCatching {
|
||||
runBlocking {
|
||||
withTimeout(BUILDER_TIMEOUT_MS) {
|
||||
engine.transcode(Uri.fromFile(input), output, request) {}
|
||||
}
|
||||
}
|
||||
}.exceptionOrNull()
|
||||
|
||||
// Two assertions, and the second is not pedantry. withTimeout raises
|
||||
// TimeoutCancellationException, and `java.util.concurrent.CancellationException` *extends*
|
||||
// IllegalStateException — so testing only the type below would call an unresumed
|
||||
// continuation a pass. A hang is the other half of this defect and every bit as bad as the
|
||||
// crash: the worker would sit holding a foreground service forever.
|
||||
assertFalse(
|
||||
"the continuation was never resumed — the failure escaped instead of being reported: " +
|
||||
"$failure",
|
||||
failure is CancellationException,
|
||||
)
|
||||
assertTrue(
|
||||
"the builder's refusal must surface as a failed job, not a dead process; got $failure",
|
||||
failure is IllegalStateException,
|
||||
)
|
||||
}
|
||||
|
||||
private fun durationMsOf(file: File): Long {
|
||||
val extractor = MediaExtractor()
|
||||
return try {
|
||||
@@ -211,5 +279,12 @@ class Media3EngineTest {
|
||||
|
||||
private companion object {
|
||||
const val TIMEOUT_SECONDS = 120L
|
||||
|
||||
/**
|
||||
* Short on purpose. Nothing is decoded or encoded on this path — the builder refuses the
|
||||
* input outright — so anything approaching this is a hang, which is what the test is
|
||||
* looking for.
|
||||
*/
|
||||
const val BUILDER_TIMEOUT_MS = 30_000L
|
||||
}
|
||||
}
|
||||
|
||||
@@ -101,7 +101,42 @@ sealed interface ConversionState {
|
||||
val mimeType: String = "",
|
||||
) : ConversionState
|
||||
data class Saved(val displayName: String) : ConversionState
|
||||
data class Failed(val message: String) : ConversionState
|
||||
|
||||
/**
|
||||
* The job, or the save that followed it, could not be finished.
|
||||
*
|
||||
* [retry] is non-null for exactly one cause: a [ConversionViewModel.save] whose copy to the
|
||||
* user's destination threw. That save deliberately keeps the staged file -- it can be the only
|
||||
* copy of an hour of transcoding -- and this is what lets the screen offer it again. Every
|
||||
* other failure leaves it null, because there is nothing staged to offer: a transcode that
|
||||
* died produced no output, and a save that found the file gone has nothing left to save.
|
||||
*
|
||||
* Nullable rather than a `SaveFailed` state of its own. What the screen does with the message
|
||||
* is identical either way, so a second variant would make every exhaustive `when` grow an arm
|
||||
* that duplicates this one.
|
||||
*
|
||||
* A view of the file, not a second owner of it -- see [PendingSave].
|
||||
*/
|
||||
data class Failed(val message: String, val retry: PendingSave? = null) : ConversionState
|
||||
}
|
||||
|
||||
/**
|
||||
* The staged output a save would target from this state, or null when there is nothing to save.
|
||||
*
|
||||
* One function for two callers that have to agree. [ConversionViewModel.save] picks the file to
|
||||
* copy with it, and `ConverterScreen` registers its `CreateDocument` contract with the MIME type
|
||||
* it returns; when those two read the state separately, a retry offered after a failed save opened
|
||||
* the dialog with the *picker's* current type instead of the finished job's -- wrong for any job
|
||||
* whose spec has been edited since, and for every reattached job, whose spec was never in these
|
||||
* settings at all.
|
||||
*
|
||||
* Top-level and `internal` rather than a member of the ViewModel, so the screen can call it
|
||||
* without one -- which is also what makes the derivation testable on the JVM.
|
||||
*/
|
||||
internal fun ConversionState.pendingSave(): PendingSave? = when (this) {
|
||||
is ConversionState.Converted -> PendingSave(staged, suggestedName, mimeType)
|
||||
is ConversionState.Failed -> retry
|
||||
else -> null
|
||||
}
|
||||
|
||||
@UnstableApi
|
||||
@@ -158,13 +193,34 @@ class ConversionViewModel @JvmOverloads constructor(
|
||||
private var observer: Job? = null
|
||||
private var activeWorkId: UUID? = null
|
||||
|
||||
/**
|
||||
* Who is allowed to write to this screen — see [ScreenOwnership] for the rule and why
|
||||
* cancelling the superseded coroutine is not one.
|
||||
*
|
||||
* Every write below that lands after a suspension point is guarded by it: the two in
|
||||
* [onInputPicked] and the one in [observe].
|
||||
*
|
||||
* [save] is the one left out, deliberately — and not because it is safe in both directions.
|
||||
* Nothing can overwrite what it writes: it is reachable only from [ConversionState.Converted]
|
||||
* or a [ConversionState.Failed] carrying its file, so the only observation that could belongs
|
||||
* to a job already in a terminal state, which will not emit again. What it can still do is
|
||||
* land on top of a [reset] taken while its copy was in flight, putting `Saved` on a screen the
|
||||
* user has just cleared. Guarding it would drop that write instead, reporting nothing for a
|
||||
* file that may genuinely have reached the user's destination. Which of those two is right is
|
||||
* a question about what the screen should offer during a save, not about this race, so it is
|
||||
* filed as issue #123 rather than decided here in passing.
|
||||
*/
|
||||
private val ownership = ScreenOwnership()
|
||||
|
||||
/**
|
||||
* The staged output this ViewModel is responsible for deleting.
|
||||
*
|
||||
* A field rather than something read back out of [_state], because the state machine
|
||||
* cannot answer the question on the path that needs it most: a failed [save] lands on
|
||||
* [ConversionState.Failed], which carries a message and no file at all. By then the
|
||||
* only remaining reference would have been lost.
|
||||
* A field rather than something read back out of [_state], and still one now that
|
||||
* [ConversionState.Failed] carries a [PendingSave] after a failed [save]. That handle is a
|
||||
* view for the screen to offer a retry through; this one is the single reference [reset]
|
||||
* deletes through, and keeping the two apart is what stops a second owner appearing. Reading
|
||||
* the file back out of the state machine instead would mean trusting every state that has no
|
||||
* file -- `Idle`, `Saved`, a transcode failure -- to say so.
|
||||
*/
|
||||
private var pendingStaged: File? = null
|
||||
|
||||
@@ -207,6 +263,10 @@ class ConversionViewModel @JvmOverloads constructor(
|
||||
* `Data` — see [ConversionState.Converted].
|
||||
*/
|
||||
private fun reattach() {
|
||||
// Read before the launch, and before the query it is about to suspend in. This is the
|
||||
// claim the answer will belong to: anything the user does from here on supersedes it, and
|
||||
// reading it on the far side of the query would read whatever superseded it instead.
|
||||
val token = ownership.current
|
||||
viewModelScope.launch {
|
||||
val reattachment = Reattachment.choose(
|
||||
workManager.jobSnapshots(
|
||||
@@ -217,8 +277,17 @@ class ConversionViewModel @JvmOverloads constructor(
|
||||
|
||||
// The query suspends, so by now the user may have picked a file or started a
|
||||
// conversion of their own. Either owns the screen; reattaching over it would throw
|
||||
// away what they just did. Both this check and the assignment below run on the main
|
||||
// dispatcher with no suspension point between them, so nothing can interleave.
|
||||
// away what they just did.
|
||||
//
|
||||
// This catches a pick that has already *landed*, and only that. It used to claim that
|
||||
// "both this check and the assignment below run on the main dispatcher with no
|
||||
// suspension point between them, so nothing can interleave" — which was the exact
|
||||
// opposite of what happens. There is no assignment below. There is observe(), which
|
||||
// launches a *separate* coroutine that must suspend on `collect` before it can write
|
||||
// anything, so the check happens at one moment and the write lands at another with a
|
||||
// whole pick able to fit in between. That was issue #49, and believing this comment is
|
||||
// why it read as flaky CI for two days. What actually holds the line is the token
|
||||
// observe() carries: see [ScreenOwnership].
|
||||
if (_state.value !is ConversionState.Idle || activeWorkId != null) return@launch
|
||||
|
||||
// Only a job that is the sole explanation for its staged file gets to name the input.
|
||||
@@ -244,7 +313,7 @@ class ConversionViewModel @JvmOverloads constructor(
|
||||
activeWorkId = reattachment.job.id
|
||||
// No initial state of our own: the flow's first emission carries the job's real
|
||||
// state, so observe() maps it exactly as it would for a conversion started here.
|
||||
observe(reattachment.job.id, input, cancelled = ConversionState.Idle)
|
||||
observe(reattachment.job.id, input, cancelled = ConversionState.Idle, token = token)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -260,25 +329,34 @@ class ConversionViewModel @JvmOverloads constructor(
|
||||
fun setQuality(quality: QualityTier) = _settings.update { it.copy(quality = quality) }
|
||||
fun setEnginePreference(preference: EnginePreference) = _settings.update { it.copy(enginePreference = preference) }
|
||||
|
||||
/**
|
||||
* The tap is the claim, which is why [ScreenOwnership.claim] is called here and not inside the
|
||||
* `launch`. A claim made in the coroutine would only be immediate for as long as
|
||||
* `Dispatchers.Main.immediate` happened to run it inline, and a deferred claim leaves the same
|
||||
* gap this closes: it is the difference between the user owning the screen from the moment
|
||||
* they tapped and owning it from whenever their coroutine got around to running.
|
||||
*/
|
||||
fun onInputPicked(uri: Uri) {
|
||||
val token = ownership.claim()
|
||||
viewModelScope.launch {
|
||||
// Both the metadata query and the probe touch disk, and the probe spawns FFprobe.
|
||||
// Neither belongs on the main thread.
|
||||
val file = withContext(pickDispatcher) { InputQuery.describe(getApplication(), uri) }
|
||||
// Every write below the hop above is guarded, this one included: two picks in quick
|
||||
// succession suspend here together, and without this the slower one would land last
|
||||
// and put the file the user did not choose on screen.
|
||||
if (!ownership.stillHeldBy(token)) return@launch
|
||||
// Show the file as soon as its name and size are known. Probing now runs FFprobe on
|
||||
// every pick, which is a native process spawn, and making the whole screen wait on it
|
||||
// would read as the app having ignored the tap.
|
||||
_state.value = ConversionState.Ready(file)
|
||||
|
||||
val probe = withContext(pickDispatcher) { probeOrUnreadable(uri) }
|
||||
// Only fill in the probe if the user has not moved on in the meantime.
|
||||
_state.update { current ->
|
||||
if (current is ConversionState.Ready && current.input.uri == uri) {
|
||||
ConversionState.Ready(file.copy(probe = probe))
|
||||
} else {
|
||||
current
|
||||
}
|
||||
}
|
||||
// Only fill in the probe if the user has not moved on in the meantime. The claim is
|
||||
// what says whether they have -- it covers a second pick of the same URI, which a
|
||||
// comparison of URIs cannot, and every state a later claim could have written.
|
||||
if (!ownership.stillHeldBy(token)) return@launch
|
||||
_state.value = ConversionState.Ready(file.copy(probe = probe))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -329,10 +407,13 @@ class ConversionViewModel @JvmOverloads constructor(
|
||||
quality = settings.quality,
|
||||
enginePreference = settings.enginePreference,
|
||||
)
|
||||
// Tapping Convert claims the screen for this job, which is what supersedes the pick's
|
||||
// still-in-flight probe and any reattachment that has not finished asking.
|
||||
val token = ownership.claim()
|
||||
activeWorkId = request.id
|
||||
workManager.enqueue(request)
|
||||
_state.value = ConversionState.Converting(input, 0)
|
||||
observe(request.id, input)
|
||||
observe(request.id, input, token = token)
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -340,12 +421,27 @@ class ConversionViewModel @JvmOverloads constructor(
|
||||
* picked file, ready to convert again. For one picked up by [reattach] there is no picked
|
||||
* file — the URI that job holds belongs to a process that no longer exists — so it lands
|
||||
* on Idle instead, rather than offering a Convert button over a file nothing can open.
|
||||
* @param token the claim this observation belongs to. Nothing here can write until `collect`
|
||||
* has resumed with a `WorkInfo`, which is some time after the caller decided to observe, so
|
||||
* the claim is checked again at the last possible moment rather than trusted from then. This
|
||||
* is issue #49's fix and the only thing standing between a superseded observation and the
|
||||
* user's screen — see [ScreenOwnership].
|
||||
*/
|
||||
private fun observe(id: UUID, input: InputFile, cancelled: ConversionState = ConversionState.Ready(input)) {
|
||||
private fun observe(
|
||||
id: UUID,
|
||||
input: InputFile,
|
||||
cancelled: ConversionState = ConversionState.Ready(input),
|
||||
token: Long,
|
||||
) {
|
||||
observer?.cancel()
|
||||
observer = viewModelScope.launch {
|
||||
workManager.getWorkInfoByIdFlow(id).collect { info ->
|
||||
if (info == null) return@collect
|
||||
// Ahead of the `when`, not merely ahead of the assignment: the SUCCEEDED branch
|
||||
// takes ownership of the staged file, and a superseded observation must not do
|
||||
// that either. The state and `pendingStaged` are meant to refer to the same file
|
||||
// or to no file, and this is where that stays true.
|
||||
if (!ownership.stillHeldBy(token)) return@collect
|
||||
_state.value = when (info.state) {
|
||||
WorkInfo.State.RUNNING -> ConversionState.Converting(
|
||||
input,
|
||||
@@ -426,35 +522,50 @@ class ConversionViewModel @JvmOverloads constructor(
|
||||
/**
|
||||
* Copies the staged result out to the destination the user picked.
|
||||
*
|
||||
* Reached from [ConversionState.Converted] and again from a [ConversionState.Failed] that an
|
||||
* earlier save left carrying its file. [pendingSave] is what makes those one call rather than
|
||||
* two, so a retry cannot drift from the first attempt in what it copies or what it calls it.
|
||||
*
|
||||
* The existence check is not redundant with the one reattachment already made. That one ran
|
||||
* inside a tag query which, for a result offered on launch, can be hours older than the tap —
|
||||
* and `cacheDir` is exactly the directory the OS empties when it wants space, which is also
|
||||
* what the sweep does to anything a day old. Without it the file's absence arrived as
|
||||
* `staged.inputStream()` throwing, and `e.message` put a raw ENOENT path on screen.
|
||||
* `staged.inputStream()` throwing, and `e.message` put a raw ENOENT path on screen. A retry
|
||||
* meets that same check a second time, which is the point of reusing it here.
|
||||
*/
|
||||
fun save(destination: Uri) {
|
||||
val converted = _state.value as? ConversionState.Converted ?: return
|
||||
if (!converted.staged.isFile) {
|
||||
val pending = _state.value.pendingSave() ?: return
|
||||
if (!pending.staged.isFile) {
|
||||
// No retry handle: the file such a state would offer again is exactly the one that
|
||||
// has gone, so carrying it would put a button on screen that cannot do anything.
|
||||
_state.value = ConversionState.Failed(STAGED_FILE_GONE_MESSAGE)
|
||||
return
|
||||
}
|
||||
viewModelScope.launch {
|
||||
runCatching {
|
||||
withContext(Dispatchers.IO) {
|
||||
publisher.publish(converted.staged, destination)
|
||||
converted.staged.delete()
|
||||
publisher.publish(pending.staged, destination)
|
||||
pending.staged.delete()
|
||||
}
|
||||
}.onSuccess {
|
||||
// publish() already deleted it; nothing left to clean up.
|
||||
pendingStaged = null
|
||||
_state.value = ConversionState.Saved(converted.suggestedName)
|
||||
_state.value = ConversionState.Saved(pending.suggestedName)
|
||||
}.onFailure { e ->
|
||||
// Deliberately NOT cleared. A failed save may mean the staged file is the
|
||||
// only copy of an hour of transcoding, and the user's destination did not
|
||||
// receive it -- deleting here would destroy the work to tidy up a cache
|
||||
// directory. It stays collectable: by a later reset(), or by the sweep once
|
||||
// it is old enough to be certain nobody is coming back for it.
|
||||
_state.value = ConversionState.Failed(e.message ?: "Could not save the file.")
|
||||
//
|
||||
// `pending` rides on the state so the screen can offer that file again. It used
|
||||
// to live only in `pendingStaged`, where nothing on screen could reach it -- so
|
||||
// the single button this branch rendered was "Start over", which deletes the very
|
||||
// file the paragraph above goes out of its way to keep. It is `pending` rather
|
||||
// than a fresh handle for the second failure's sake: a retry that fails again
|
||||
// lands back here still carrying the file, not on a bare Failed that would take
|
||||
// the offer away.
|
||||
_state.value = ConversionState.Failed(e.message ?: "Could not save the file.", pending)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -468,8 +579,18 @@ class ConversionViewModel @JvmOverloads constructor(
|
||||
* cancelled with [viewModelScope] if the Activity finishes first, so it is a best
|
||||
* effort rather than a guarantee. `OutputPublisher.sweepStaging` is the backstop for
|
||||
* the times it does not run.
|
||||
*
|
||||
* **It still deletes from a [ConversionState.Failed] carrying a [PendingSave], and that is a
|
||||
* decision rather than something inherited.** Deletion is acceptable there only because the
|
||||
* alternative was offered first: the screen puts "Try saving again" directly above this
|
||||
* button, so reaching it is the user saying the work is not worth keeping. Until that button
|
||||
* existed, this delete was the only thing a failed save could lead to — which was the defect.
|
||||
*/
|
||||
fun reset() {
|
||||
// Start over is a claim like any other. The cancel below is a request honoured at the next
|
||||
// suspension point, so a collector already on its way to a write has nothing left to
|
||||
// honour it at; the claim is what actually stops that write landing on top of Idle.
|
||||
ownership.claim()
|
||||
observer?.cancel()
|
||||
observer = null
|
||||
activeWorkId = null
|
||||
|
||||
@@ -73,8 +73,11 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode
|
||||
// stands: some providers rewrite a document's extension to match it, so an MP3 offered as
|
||||
// video/webm can arrive with the wrong one. Read straight off the collected state, so this
|
||||
// recomposes because it depends on that rather than because an unrelated line happens to.
|
||||
// Through pendingSave() rather than a cast to Converted, so a retry offered after a failed
|
||||
// save opens the dialog with the type its first attempt used -- the cast answered null for a
|
||||
// Failed, and the fallback below is the current picker, which a reattached job never set.
|
||||
// Remembered against the type so the launcher re-registers only when it actually changes.
|
||||
val destinationMime = (state as? ConversionState.Converted)?.mimeType ?: settings.spec.mimeType
|
||||
val destinationMime = state.pendingSave()?.mimeType ?: settings.spec.mimeType
|
||||
val chooseDestination = rememberLauncherForActivityResult(
|
||||
remember(destinationMime) { ActivityResultContracts.CreateDocument(destinationMime) },
|
||||
) { uri -> uri?.let(viewModel::save) }
|
||||
@@ -325,13 +328,36 @@ internal fun ConverterScreenContent(
|
||||
color = MaterialTheme.colorScheme.error,
|
||||
style = MaterialTheme.typography.bodyMedium,
|
||||
)
|
||||
Button(
|
||||
onClick = actions.onReset,
|
||||
modifier = Modifier
|
||||
.fillMaxWidth()
|
||||
.height(PrimaryButtonHeight)
|
||||
.testTag(TestTags.START_OVER),
|
||||
) { Text("Start over") }
|
||||
val retry = s.retry
|
||||
if (retry == null) {
|
||||
// Nothing was staged, so "Start over" is the whole of what is on
|
||||
// offer and stays the primary button.
|
||||
Button(
|
||||
onClick = actions.onReset,
|
||||
modifier = Modifier
|
||||
.fillMaxWidth()
|
||||
.height(PrimaryButtonHeight)
|
||||
.testTag(TestTags.START_OVER),
|
||||
) { Text("Start over") }
|
||||
} else {
|
||||
Button(
|
||||
onClick = { actions.onSave(retry.suggestedName) },
|
||||
modifier = Modifier
|
||||
.fillMaxWidth()
|
||||
.height(PrimaryButtonHeight)
|
||||
.testTag(TestTags.RETRY_SAVE),
|
||||
) { Text("Try saving again") }
|
||||
// Start over still deletes the file this state is carrying, and that
|
||||
// is deliberate: `reset()` is what stops a full-size output sitting in
|
||||
// cache until the sweep. What makes the delete acceptable is the
|
||||
// button above it. Deletion is the user's choice only once the
|
||||
// alternative has been offered -- and until that button existed, this
|
||||
// one was the only thing a failed save could lead to.
|
||||
OutlinedButton(
|
||||
onClick = actions.onReset,
|
||||
modifier = Modifier.fillMaxWidth().testTag(TestTags.START_OVER),
|
||||
) { Text("Start over") }
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -68,42 +68,64 @@ class Media3Engine(private val context: Context) : HardwareTranscoder {
|
||||
): Unit = suspendCancellableCoroutine { cont ->
|
||||
val plan = CopyPlanner.plan(request.spec, request.probe)
|
||||
handler.post {
|
||||
val transformer = runCatching { buildTransformer(plan, cont) }
|
||||
.getOrElse {
|
||||
cont.resumeWithException(it)
|
||||
return@post
|
||||
}
|
||||
|
||||
// Dropping the tracks the target does not have is what stops an audio-only export
|
||||
// from carrying a re-encoded video track. Without setRemoveVideo, asking for M4A
|
||||
// produced an HEVC stream in a file named .m4a.
|
||||
val item = EditedMediaItem.Builder(MediaItem.fromUri(input))
|
||||
.setRemoveVideo(plan.video == VideoPlan.Drop)
|
||||
.setRemoveAudio(plan.audio == AudioPlan.Drop)
|
||||
.build()
|
||||
|
||||
// A Composition is the only way to ask for transmuxing; the plain
|
||||
// start(EditedMediaItem, path) overload always re-encodes. This is the remux path.
|
||||
val composition = Composition.Builder(EditedMediaItemSequence.Builder(item).build())
|
||||
.setTransmuxVideo(plan.video == VideoPlan.Copy)
|
||||
.setTransmuxAudio(plan.audio == AudioPlan.Copy)
|
||||
.build()
|
||||
|
||||
cont.invokeOnCancellation {
|
||||
// cancel() has the same single-thread requirement as start().
|
||||
handler.post { runCatching { transformer.cancel() } }
|
||||
}
|
||||
|
||||
runCatching { transformer.start(composition, output.absolutePath) }
|
||||
.onFailure {
|
||||
cont.resumeWithException(it)
|
||||
return@post
|
||||
}
|
||||
|
||||
pollProgress(transformer, cont, onProgress)
|
||||
// One guard around the whole body, deliberately.
|
||||
//
|
||||
// This used to be two narrow ones — around `buildTransformer` and around
|
||||
// `transformer.start` — with the two Media3 builders sitting unguarded between them.
|
||||
// On this thread that is not a small gap: nothing here has a caller to throw back to,
|
||||
// so an escaping exception reaches the HandlerThread's uncaught handler and takes the
|
||||
// process down, while [cont] is never resumed either way. `EditedMediaItem.Builder`
|
||||
// does exactly that for a plan that drops both tracks
|
||||
// ("Audio and video cannot both be removed"), which a queued job can still carry.
|
||||
// Widening the guard costs nothing on success and turns every such refusal into a
|
||||
// failed job with a reason.
|
||||
runCatching { startExport(input, output, plan, cont, onProgress) }
|
||||
.onFailure { if (cont.isActive) cont.resumeWithException(it) }
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Builds the export and hands it to Transformer. Runs on the HandlerThread; may throw.
|
||||
*
|
||||
* Everything Transformer's single-thread contract covers lives here, so that the caller has
|
||||
* exactly one place to catch. Returning normally means the export is running and [cont] belongs
|
||||
* to the listener; throwing means it never started and the caller owns resuming.
|
||||
*/
|
||||
private fun startExport(
|
||||
input: Uri,
|
||||
output: File,
|
||||
plan: ConversionPlan,
|
||||
cont: CancellableContinuation<Unit>,
|
||||
onProgress: (Int) -> Unit,
|
||||
) {
|
||||
val transformer = buildTransformer(plan, cont)
|
||||
|
||||
// Dropping the tracks the target does not have is what stops an audio-only export
|
||||
// from carrying a re-encoded video track. Without setRemoveVideo, asking for M4A
|
||||
// produced an HEVC stream in a file named .m4a.
|
||||
val item = EditedMediaItem.Builder(MediaItem.fromUri(input))
|
||||
.setRemoveVideo(plan.video == VideoPlan.Drop)
|
||||
.setRemoveAudio(plan.audio == AudioPlan.Drop)
|
||||
.build()
|
||||
|
||||
// A Composition is the only way to ask for transmuxing; the plain
|
||||
// start(EditedMediaItem, path) overload always re-encodes. This is the remux path.
|
||||
val composition = Composition.Builder(EditedMediaItemSequence.Builder(item).build())
|
||||
.setTransmuxVideo(plan.video == VideoPlan.Copy)
|
||||
.setTransmuxAudio(plan.audio == AudioPlan.Copy)
|
||||
.build()
|
||||
|
||||
// Registered before start(), so a cancellation racing the export always finds a
|
||||
// transformer to cancel.
|
||||
cont.invokeOnCancellation {
|
||||
// cancel() has the same single-thread requirement as start().
|
||||
handler.post { runCatching { transformer.cancel() } }
|
||||
}
|
||||
|
||||
transformer.start(composition, output.absolutePath)
|
||||
pollProgress(transformer, cont, onProgress)
|
||||
}
|
||||
|
||||
/**
|
||||
* @throws IllegalArgumentException if [plan] names a container Media3 cannot mux. That is a
|
||||
* routing bug rather than a runtime condition — [org.libremediaconverter.model.ConversionRouter]
|
||||
|
||||
@@ -23,6 +23,24 @@ const val STAGED_FILE_GONE_MESSAGE: String =
|
||||
"The finished file is no longer in the cache, so there is nothing left to save. " +
|
||||
"Start over to make it again."
|
||||
|
||||
/**
|
||||
* A staged file that is still there to be saved, and everything the save dialog needs to offer it.
|
||||
*
|
||||
* The three travel together because a save cannot be repeated without all of them: the file to
|
||||
* copy, the name to suggest, and the MIME type `CreateDocument` has to be registered with. None of
|
||||
* them can be rederived from the pickers once the job is over -- they come from the job's own
|
||||
* output `Data`, and a reattached job's spec was never in the current settings at all.
|
||||
*
|
||||
* Kept next to [STAGED_FILE_GONE_MESSAGE] for the same reason it is: both ViewModels need it and
|
||||
* staging is what it is about.
|
||||
*
|
||||
* **A view of the staged file, never an owner of it.** The delete still runs through each
|
||||
* ViewModel's own `pendingStaged` field, so a state carrying one of these can be dropped without
|
||||
* losing the only reference -- which is what keeps "a `Failed` that carries a file" from being a
|
||||
* new way to leak one.
|
||||
*/
|
||||
data class PendingSave(val staged: File, val suggestedName: String, val mimeType: String)
|
||||
|
||||
/**
|
||||
* Staging and publication of conversion output.
|
||||
*
|
||||
|
||||
@@ -0,0 +1,57 @@
|
||||
package org.libremediaconverter.convert
|
||||
|
||||
/**
|
||||
* Which of the things writing to a screen is still allowed to.
|
||||
*
|
||||
* Both ViewModels are a state machine written to from several coroutines that each suspend before
|
||||
* they write: a pick hops to a dispatcher for the metadata query, a reattachment hops for the tag
|
||||
* query, and an observation of a WorkManager job cannot write at all until its `collect` has
|
||||
* resumed with a `WorkInfo`. Whoever resumes last wins, which is how issue #49 let a finished job
|
||||
* from an earlier session take a screen the user had already picked a file on.
|
||||
*
|
||||
* The rule this makes enforceable is one line: **every write that lands after a suspension point
|
||||
* checks the claim it was made under, and drops itself if that claim has been superseded.** The
|
||||
* claim is taken synchronously, when the user acts; the check happens immediately before the
|
||||
* write. Superseded work is *dropped*, not reordered — a dropped write cannot come back later.
|
||||
*
|
||||
* Cancelling the superseded coroutine is not a substitute and was never going to be. `Job.cancel`
|
||||
* is a request, honoured at the next suspension point; a collector that has already resumed and is
|
||||
* on its way to `_state.value = …` has no suspension point left to honour it at, so the write
|
||||
* lands anyway. Cancellation also cannot help at all in the case #49 actually reported, where
|
||||
* nothing supersedes the observation until after it has been launched. Both ViewModels still
|
||||
* cancel their old observer, because leaving a collector running is a leak — but the guarantee
|
||||
* does not rest on it.
|
||||
*
|
||||
* **Confined to the main dispatcher, and that confinement is the atomicity argument.** Every
|
||||
* claim and every check runs there, with no suspension point between a check and the write it
|
||||
* guards, so a claim can never land between the two. Nothing here is synchronized and nothing is
|
||||
* `@Volatile`: making the field visible across threads would invite exactly the off-main use this
|
||||
* cannot support, and would replace an argument that holds with one that only looks like it does.
|
||||
*/
|
||||
internal class ScreenOwnership {
|
||||
|
||||
private var claims = 0L
|
||||
|
||||
/**
|
||||
* The claim in force now.
|
||||
*
|
||||
* Read by work that is about to suspend and will want to know, when it comes back, whether
|
||||
* the screen it was reading is still the screen it is writing to. Read it *before* the
|
||||
* suspension, not after — reading it afterwards would return whatever claim superseded it,
|
||||
* which is the bug rather than the check for it.
|
||||
*/
|
||||
val current: Long get() = claims
|
||||
|
||||
/**
|
||||
* Takes the screen, invalidating every write still in flight under an older claim.
|
||||
*
|
||||
* Called synchronously from the user's action rather than from inside the coroutine it
|
||||
* starts. A claim made inside a `launch` is only immediate while the dispatcher happens to
|
||||
* run it inline, and a deferred claim is no claim at all: it would leave the same gap this
|
||||
* exists to close.
|
||||
*/
|
||||
fun claim(): Long = ++claims
|
||||
|
||||
/** Whether [token] is still the claim in force, and may therefore write. */
|
||||
fun stillHeldBy(token: Long): Boolean = token == claims
|
||||
}
|
||||
@@ -46,9 +46,10 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod
|
||||
|
||||
// The contract's MIME type comes from the finished job rather than from a literal: some
|
||||
// providers rewrite a document's extension to match it, so naming MP4 for a join that is not
|
||||
// one can hand the user a file the extension lies about. Remembered against that type so the
|
||||
// launcher re-registers only when it actually changes.
|
||||
val destinationMime = (state as? JoinState.Joined)?.mimeType ?: ConcatWorker.DEFAULT_FORMAT.mimeType
|
||||
// one can hand the user a file the extension lies about. Through pendingSave() rather than a
|
||||
// cast to Joined, so a retry after a failed save opens with the type its first attempt used.
|
||||
// Remembered against that type so the launcher re-registers only when it actually changes.
|
||||
val destinationMime = state.pendingSave()?.mimeType ?: ConcatWorker.DEFAULT_FORMAT.mimeType
|
||||
val chooseDestination = rememberLauncherForActivityResult(
|
||||
remember(destinationMime) { ActivityResultContracts.CreateDocument(destinationMime) },
|
||||
) { uri -> uri?.let(viewModel::save) }
|
||||
@@ -226,13 +227,32 @@ internal fun JoinScreenContent(state: JoinState, actions: JoinActions, modifier:
|
||||
color = MaterialTheme.colorScheme.error,
|
||||
style = MaterialTheme.typography.bodyMedium,
|
||||
)
|
||||
Button(
|
||||
onClick = actions.onReset,
|
||||
modifier = Modifier
|
||||
.fillMaxWidth()
|
||||
.height(PrimaryButtonHeight)
|
||||
.testTag(TestTags.START_OVER),
|
||||
) { Text("Start over") }
|
||||
val retry = s.retry
|
||||
if (retry == null) {
|
||||
// Nothing staged, so "Start over" is all there is and stays primary.
|
||||
Button(
|
||||
onClick = actions.onReset,
|
||||
modifier = Modifier
|
||||
.fillMaxWidth()
|
||||
.height(PrimaryButtonHeight)
|
||||
.testTag(TestTags.START_OVER),
|
||||
) { Text("Start over") }
|
||||
} else {
|
||||
Button(
|
||||
onClick = { actions.onSave(retry.suggestedName) },
|
||||
modifier = Modifier
|
||||
.fillMaxWidth()
|
||||
.height(PrimaryButtonHeight)
|
||||
.testTag(TestTags.RETRY_SAVE),
|
||||
) { Text("Try saving again") }
|
||||
// Start over still deletes the carried file, for the reason the
|
||||
// converter screen writes out next to the same pair of buttons:
|
||||
// the delete is a choice only once the alternative is on screen.
|
||||
OutlinedButton(
|
||||
onClick = actions.onReset,
|
||||
modifier = Modifier.fillMaxWidth().testTag(TestTags.START_OVER),
|
||||
) { Text("Start over") }
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -18,7 +18,9 @@ import kotlinx.coroutines.withContext
|
||||
import org.libremediaconverter.convert.ConversionDependencies
|
||||
import org.libremediaconverter.convert.InputFile
|
||||
import org.libremediaconverter.convert.InputQuery
|
||||
import org.libremediaconverter.convert.PendingSave
|
||||
import org.libremediaconverter.convert.STAGED_FILE_GONE_MESSAGE
|
||||
import org.libremediaconverter.convert.ScreenOwnership
|
||||
import org.libremediaconverter.model.ConcatStrategy
|
||||
import org.libremediaconverter.work.ConcatWorker
|
||||
import org.libremediaconverter.work.JobTags
|
||||
@@ -44,7 +46,30 @@ sealed interface JoinState {
|
||||
val mimeType: String,
|
||||
) : JoinState
|
||||
data class Saved(val displayName: String) : JoinState
|
||||
data class Failed(val message: String) : JoinState
|
||||
|
||||
/**
|
||||
* The join, or the save that followed it, could not be finished.
|
||||
*
|
||||
* [retry] is non-null for exactly one cause, and for the same reason as on
|
||||
* `ConversionState.Failed`: a [JoinViewModel.save] whose copy to the user's destination threw
|
||||
* keeps the staged file, and this is what lets the screen offer it again. Every other failure
|
||||
* leaves it null — a join that died produced no output, and a save that found the file gone
|
||||
* has nothing left to save.
|
||||
*/
|
||||
data class Failed(val message: String, val retry: PendingSave? = null) : JoinState
|
||||
}
|
||||
|
||||
/**
|
||||
* The staged output a save would target from this state, or null when there is nothing to save.
|
||||
*
|
||||
* The join tab's half of `ConversionState.pendingSave`, and it exists for the same reason: `save`
|
||||
* and `JoinScreen`'s `CreateDocument` registration both have to answer this question, and answering
|
||||
* it twice is how a retry ends up opening the dialog with a type the finished job never chose.
|
||||
*/
|
||||
internal fun JoinState.pendingSave(): PendingSave? = when (this) {
|
||||
is JoinState.Joined -> PendingSave(staged, suggestedName, mimeType)
|
||||
is JoinState.Failed -> retry
|
||||
else -> null
|
||||
}
|
||||
|
||||
@UnstableApi
|
||||
@@ -52,6 +77,15 @@ class JoinViewModel @JvmOverloads constructor(
|
||||
app: Application,
|
||||
/** Where [reset] runs its delete. See the same parameter on `ConversionViewModel`. */
|
||||
private val cleanupDispatcher: CoroutineDispatcher = Dispatchers.IO,
|
||||
/**
|
||||
* Where the metadata query behind a pick runs. See the same parameter on `ConversionViewModel`.
|
||||
*
|
||||
* The join side had no such seam, and the gap was not cosmetic: the one write `onInputsPicked`
|
||||
* makes lands *after* this hop, so a test that wants to ask what happens while a pick is still
|
||||
* in flight had no way to hold one there. Issue #49's race is exactly that question, and it
|
||||
* went unasked on this side for as long as the dispatcher was a literal.
|
||||
*/
|
||||
private val pickDispatcher: CoroutineDispatcher = Dispatchers.IO,
|
||||
) : AndroidViewModel(app) {
|
||||
|
||||
private val workManager = WorkManager.getInstance(app)
|
||||
@@ -66,13 +100,28 @@ class JoinViewModel @JvmOverloads constructor(
|
||||
private var observer: Job? = null
|
||||
private var activeWorkId: UUID? = null
|
||||
|
||||
/**
|
||||
* Who is allowed to write to this screen -- see [ScreenOwnership], which carries the rule and
|
||||
* the reason cancelling the superseded coroutine is not one.
|
||||
*
|
||||
* The convert side had issue #49 reported against it four times in two days; this side has the
|
||||
* identical shape and was never reported, because nothing was watching. Every write below that
|
||||
* lands after a suspension point is guarded: the one in [onInputsPicked] and the one in
|
||||
* [observe]. [save] is the one left out, deliberately and with the same limit its counterpart
|
||||
* in `ConversionViewModel` spells out: nothing can overwrite what it writes, but it can still
|
||||
* land on top of a [reset] taken while its copy was in flight. Which way that should go is a
|
||||
* question about the save screen rather than about this race -- issue #123.
|
||||
*/
|
||||
private val ownership = ScreenOwnership()
|
||||
|
||||
/**
|
||||
* The staged output this ViewModel is responsible for deleting.
|
||||
*
|
||||
* Held here rather than read back out of [_state] for the same reason as in
|
||||
* `ConversionViewModel`: a failed [save] lands on [JoinState.Failed], which carries a
|
||||
* message and no file, so the state machine cannot answer this on the one path that
|
||||
* most needs it.
|
||||
* `ConversionViewModel`, and still held here now that [JoinState.Failed] carries a
|
||||
* [PendingSave] after a failed [save]: that handle is a view for the screen to offer a retry
|
||||
* through, this one is the single reference [reset] deletes through, and keeping the two
|
||||
* apart is what stops a second owner of the file appearing.
|
||||
*/
|
||||
private var pendingStaged: File? = null
|
||||
|
||||
@@ -91,6 +140,10 @@ class JoinViewModel @JvmOverloads constructor(
|
||||
* the rules about which job and why.
|
||||
*/
|
||||
private fun reattach() {
|
||||
// Read before the launch, and before the query it is about to suspend in: this is the
|
||||
// claim the answer belongs to. Reading it on the far side of the query would read whatever
|
||||
// superseded it, which is the bug rather than the check for it.
|
||||
val token = ownership.current
|
||||
viewModelScope.launch {
|
||||
val reattachment = Reattachment.choose(
|
||||
workManager.jobSnapshots(
|
||||
@@ -100,8 +153,15 @@ class JoinViewModel @JvmOverloads constructor(
|
||||
) ?: return@launch
|
||||
|
||||
// The query suspends, so the user may have picked files or started a join in the
|
||||
// meantime. Theirs wins. No suspension point between this check and the assignment
|
||||
// below, and both run on the main dispatcher, so nothing can interleave.
|
||||
// meantime. Theirs wins.
|
||||
//
|
||||
// This catches a pick that has already *landed*, and only that. It used to claim there
|
||||
// was "no suspension point between this check and the assignment below", which was the
|
||||
// opposite of what happens: there is no assignment below, only observe(), which
|
||||
// launches a separate coroutine that cannot write until its `collect` resumes. The
|
||||
// check happens at one moment and the write lands at another, with a whole pick able
|
||||
// to fit in between -- issue #49. The token observe() carries is what holds that line;
|
||||
// see [ScreenOwnership].
|
||||
if (_state.value !is JoinState.Idle || activeWorkId != null) return@launch
|
||||
|
||||
// Joins used to stage under one constant name, so two finished joins always reported
|
||||
@@ -121,19 +181,29 @@ class JoinViewModel @JvmOverloads constructor(
|
||||
InputFile(Uri.EMPTY, "", sizeBytes = null)
|
||||
}
|
||||
activeWorkId = reattachment.job.id
|
||||
observe(reattachment.job.id, inputs, cancelled = JoinState.Idle)
|
||||
observe(reattachment.job.id, inputs, cancelled = JoinState.Idle, token = token)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The tap is the claim, which is why it is taken here rather than inside the `launch` -- and
|
||||
* above the early return, so the refusal below is covered by it too. A claim made in the
|
||||
* coroutine is only immediate while `Dispatchers.Main.immediate` happens to run it inline, and
|
||||
* a deferred claim leaves exactly the gap this closes.
|
||||
*/
|
||||
fun onInputsPicked(uris: List<Uri>) {
|
||||
val token = ownership.claim()
|
||||
if (uris.size < 2) {
|
||||
_state.value = JoinState.Failed("Pick at least two files to join.")
|
||||
return
|
||||
}
|
||||
viewModelScope.launch {
|
||||
val files = withContext(Dispatchers.IO) {
|
||||
val files = withContext(pickDispatcher) {
|
||||
uris.map { InputQuery.describe(getApplication(), it) }
|
||||
}
|
||||
// Guarded like every other write that lands after a hop: two picks in quick succession
|
||||
// suspend here together, and the slower one would otherwise land last.
|
||||
if (!ownership.stillHeldBy(token)) return@launch
|
||||
_state.value = JoinState.Ready(files)
|
||||
}
|
||||
}
|
||||
@@ -147,10 +217,13 @@ class JoinViewModel @JvmOverloads constructor(
|
||||
// did answer would hand the space check a lower bound it would read as a total.
|
||||
totalBytes = InputQuery.total(inputs.map { it.sizeBytes }),
|
||||
)
|
||||
// Tapping Join claims the screen for this job, superseding any reattachment that has not
|
||||
// finished asking.
|
||||
val token = ownership.claim()
|
||||
activeWorkId = request.id
|
||||
workManager.enqueue(request)
|
||||
_state.value = JoinState.Joining(inputs)
|
||||
observe(request.id, inputs)
|
||||
observe(request.id, inputs, token = token)
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -158,12 +231,25 @@ class JoinViewModel @JvmOverloads constructor(
|
||||
* files, ready to join again. For one picked up by [reattach] there are no picked files —
|
||||
* what that job holds are URIs granted to a process that no longer exists — so it lands on
|
||||
* Idle rather than offering to re-join files nothing can open.
|
||||
* @param token the claim this observation belongs to. Nothing here can write until `collect`
|
||||
* has resumed with a `WorkInfo`, which is some time after the caller decided to observe, so
|
||||
* the claim is checked again at the last possible moment rather than trusted from then. See
|
||||
* [ScreenOwnership], and issue #49.
|
||||
*/
|
||||
private fun observe(id: UUID, inputs: List<InputFile>, cancelled: JoinState = JoinState.Ready(inputs)) {
|
||||
private fun observe(
|
||||
id: UUID,
|
||||
inputs: List<InputFile>,
|
||||
cancelled: JoinState = JoinState.Ready(inputs),
|
||||
token: Long,
|
||||
) {
|
||||
observer?.cancel()
|
||||
observer = viewModelScope.launch {
|
||||
workManager.getWorkInfoByIdFlow(id).collect { info ->
|
||||
if (info == null) return@collect
|
||||
// Ahead of the `when`, not merely ahead of the assignment: the SUCCEEDED branch
|
||||
// takes ownership of the staged file, and a superseded observation must not do
|
||||
// that either.
|
||||
if (!ownership.stillHeldBy(token)) return@collect
|
||||
_state.value = when (info.state) {
|
||||
WorkInfo.State.RUNNING, WorkInfo.State.BLOCKED -> JoinState.Joining(inputs)
|
||||
WorkInfo.State.ENQUEUED ->
|
||||
@@ -229,29 +315,38 @@ class JoinViewModel @JvmOverloads constructor(
|
||||
* join offered by reattachment was last seen during a tag query that may be hours old, and
|
||||
* `cacheDir` is reclaimed by the OS and swept by this app. Without it the file's absence
|
||||
* reached the screen as a raw ENOENT path.
|
||||
*
|
||||
* Reached from [JoinState.Joined] and again from a [JoinState.Failed] an earlier save left
|
||||
* carrying its file; [pendingSave] is what makes those the same call.
|
||||
*/
|
||||
fun save(destination: Uri) {
|
||||
val joined = _state.value as? JoinState.Joined ?: return
|
||||
if (!joined.staged.isFile) {
|
||||
val pending = _state.value.pendingSave() ?: return
|
||||
if (!pending.staged.isFile) {
|
||||
// No retry handle -- the file it would offer again is the one that has gone.
|
||||
_state.value = JoinState.Failed(STAGED_FILE_GONE_MESSAGE)
|
||||
return
|
||||
}
|
||||
viewModelScope.launch {
|
||||
runCatching {
|
||||
withContext(Dispatchers.IO) {
|
||||
publisher.publish(joined.staged, destination)
|
||||
joined.staged.delete()
|
||||
publisher.publish(pending.staged, destination)
|
||||
pending.staged.delete()
|
||||
}
|
||||
}.onSuccess {
|
||||
// publish() already deleted it; nothing left to clean up.
|
||||
pendingStaged = null
|
||||
_state.value = JoinState.Saved(joined.suggestedName)
|
||||
_state.value = JoinState.Saved(pending.suggestedName)
|
||||
}.onFailure { e ->
|
||||
// Deliberately NOT cleared -- see the same branch in ConversionViewModel.
|
||||
// A failed save can leave the staged file as the only copy of the work, so
|
||||
// it is left for a later reset() or for the sweep to collect once its age
|
||||
// makes it certain nobody is coming back for it.
|
||||
_state.value = JoinState.Failed(e.message ?: "Could not save the file.")
|
||||
//
|
||||
// `pending` travels on the state so the screen can offer the file again rather
|
||||
// than leaving "Start over" -- which deletes it -- as the only thing on offer.
|
||||
// Passing `pending` rather than rebuilding it is what keeps a retry that fails
|
||||
// again on a carrying Failed instead of a bare one.
|
||||
_state.value = JoinState.Failed(e.message ?: "Could not save the file.", pending)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -261,8 +356,16 @@ class JoinViewModel @JvmOverloads constructor(
|
||||
*
|
||||
* Best effort, not a guarantee: the delete is cancelled with [viewModelScope] if the
|
||||
* Activity finishes first. `OutputPublisher.sweepStaging` is the backstop.
|
||||
*
|
||||
* It deletes from a [JoinState.Failed] carrying a [PendingSave] too, deliberately and for the
|
||||
* reason `ConversionViewModel.reset` writes out: the screen offers "Try saving again" above
|
||||
* this button, so deletion is what the user chose rather than all this state could do.
|
||||
*/
|
||||
fun reset() {
|
||||
// Start over is a claim like any other. The cancel below is a request honoured at the next
|
||||
// suspension point, so a collector already on its way to a write has nothing left to
|
||||
// honour it at; the claim is what stops that write landing on top of Idle.
|
||||
ownership.claim()
|
||||
observer?.cancel()
|
||||
observer = null
|
||||
activeWorkId = null
|
||||
|
||||
@@ -129,9 +129,25 @@ object ContainerCapabilities {
|
||||
}
|
||||
}
|
||||
|
||||
if (spec.videoCodec == VideoCodec.NONE && spec.audioCodec == AudioCodec.NONE) {
|
||||
// Two faces of one rule: the output would carry no tracks at all.
|
||||
//
|
||||
// The first is visible in the spec alone — NONE on both axes. The second only emerges once
|
||||
// the spec meets the probe, because [CopyPlanner] drops a video track the *input* does not
|
||||
// have no matter which codec was named for it, so "H.265 + no audio" on an MP3 plans to
|
||||
// (Drop, Drop) exactly as "None + None" does. Asking the spec alone answered the first and
|
||||
// missed the second, and the miss was not cosmetic: `EditedMediaItem.Builder` refuses that
|
||||
// composition with IllegalStateException("Audio and video cannot both be removed"), on
|
||||
// Transformer's own thread, where the user would have seen a dead app rather than a reason.
|
||||
if (spec.audioCodec == AudioCodec.NONE && (spec.videoCodec == VideoCodec.NONE || !probe.hasVideo)) {
|
||||
return Validation.Invalid(
|
||||
"This would produce an empty file — keep at least one track.",
|
||||
if (spec.videoCodec == VideoCodec.NONE) {
|
||||
"This would produce an empty file — keep at least one track."
|
||||
} else {
|
||||
// Names both halves. "No video track" alone reads as though the video setting
|
||||
// were the only thing wrong, and the user would fix that and still be stuck.
|
||||
"This file has no video track, so turning the audio off too would produce an " +
|
||||
"empty file."
|
||||
},
|
||||
suggestions(
|
||||
// Ask for both tracks back, then let repair settle what this container and
|
||||
// this input can actually give.
|
||||
@@ -162,7 +178,12 @@ object ContainerCapabilities {
|
||||
if (!probe.hasVideo) {
|
||||
return Validation.Invalid(
|
||||
"This file has no video track to copy.",
|
||||
listOf(spec.copy(videoCodec = VideoCodec.NONE)),
|
||||
// Dropping the video is the right shape of answer, but it is only half of one:
|
||||
// `spec.copy(videoCodec = NONE)` is valid exactly when the audio axis already
|
||||
// happened to be fine, and refused otherwise — a Vorbis or PCM source into MP4,
|
||||
// an MP3 into WebM. Handing it to the shared path repairs both axes and drops
|
||||
// anything that still fails, so the chip cannot lead to a second error.
|
||||
suggestions(spec.copy(videoCodec = VideoCodec.NONE), probe, exclude = spec),
|
||||
)
|
||||
}
|
||||
val source = CodecNames.videoFromName(probe.videoCodec)
|
||||
@@ -284,8 +305,12 @@ object ContainerCapabilities {
|
||||
private fun repairVideo(spec: OutputSpec, probe: InputProbe): VideoCodec {
|
||||
val container = spec.container
|
||||
if (spec.videoCodec == VideoCodec.NONE || !container.canHoldVideo) return VideoCodec.NONE
|
||||
// There is no video track to make one out of, so naming a codec would be a suggestion
|
||||
// [CopyPlanner] drops on the floor. It also read as a non-sequitur: before this line, the
|
||||
// repair offered for an MP3 was "H.264", the first codec MP4 happens to encode.
|
||||
if (!probe.hasVideo) return VideoCodec.NONE
|
||||
|
||||
val source = CodecNames.videoFromName(probe.videoCodec).takeIf { probe.hasVideo }
|
||||
val source = CodecNames.videoFromName(probe.videoCodec)
|
||||
val copyable = source != null && accepts(container, source, CodecMode.COPY)
|
||||
|
||||
return when {
|
||||
|
||||
@@ -45,6 +45,17 @@ object TestTags {
|
||||
|
||||
const val SAVE_FILE: String = "action.saveFile"
|
||||
|
||||
/**
|
||||
* The retry a `Failed` offers after a save that threw, on both screens.
|
||||
*
|
||||
* Its own tag rather than [SAVE_FILE], because the two are different claims about the screen.
|
||||
* [SAVE_FILE] is the first attempt from a finished job; this one may appear only where a staged
|
||||
* file survived a failed save. Sharing a tag would collapse "a transcode failure offers nothing
|
||||
* to save" and "a failed save offers the file again" into one query, and that first assertion
|
||||
* is the one stopping a Save button from appearing where there is nothing to save.
|
||||
*/
|
||||
const val RETRY_SAVE: String = "action.retrySave"
|
||||
|
||||
/** `ConverterScreen`. */
|
||||
object Converter {
|
||||
const val CHOOSE_FILE: String = "converter.chooseFile"
|
||||
|
||||
@@ -25,8 +25,18 @@ private val LightColorScheme = lightColorScheme(
|
||||
* Material 3 theme.
|
||||
*
|
||||
* Dynamic color (Material You) needs API 31+; minSdk is 33, so it is available
|
||||
* unconditionally and no version guard is required. It stays switchable so users can
|
||||
* opt back to the brand palette.
|
||||
* unconditionally and no version guard is required.
|
||||
*
|
||||
* [dynamicColor] has no caller. `MainActivity` is the single call site and takes the
|
||||
* default, so the parameter is always `true`, the two dynamic branches always win, and
|
||||
* [DarkColorScheme] and [LightColorScheme] are dead: nothing in the app can opt back to the
|
||||
* brand palette. `ThemeColorSchemeTest` reaches those two branches only by passing
|
||||
* [dynamicColor] explicitly -- a test doing it, not a feature.
|
||||
*
|
||||
* That is known rather than an oversight. #68 holds the choice between adding a switch,
|
||||
* deleting the dead branches together with the template palette, and replacing that palette
|
||||
* first; it is undecided, so nothing here should be read as a promise that any of them
|
||||
* happens.
|
||||
*/
|
||||
@Composable
|
||||
fun LibreMediaConverterTheme(
|
||||
|
||||
@@ -0,0 +1,67 @@
|
||||
package org.libremediaconverter.ci
|
||||
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Test
|
||||
import java.io.File
|
||||
|
||||
/**
|
||||
* That the release job still holds the one permission it needs to publish.
|
||||
*
|
||||
* `build.yml`'s `release` job declares `contents: write`, and nothing was checking it. Deleting
|
||||
* those two lines leaves actionlint clean and CodeQL silent — a *narrower* permission is not an
|
||||
* alert — and the job is `if: startsWith(github.ref, 'refs/tags/v')`, so no pull request and no
|
||||
* merge to `main` can exercise it. Measured: with the declaration removed, every gating check
|
||||
* still passes. The first thing that would notice is a release failing to publish, at the moment
|
||||
* someone is trying to cut one.
|
||||
*
|
||||
* The deletion also looks like tidying. A top-level `permissions: contents: read` now sits
|
||||
* directly above it, so a reader could reasonably take the job-level block for a duplicate. It is
|
||||
* an override, not a duplicate, and a comment saying so is not a check.
|
||||
*
|
||||
* `BackupExclusionsTest` is the precedent: a file that is configuration rather than code, load
|
||||
* bearing, and unguarded because nothing compiles it.
|
||||
*
|
||||
* **What this pins, and what it does not.** It asserts the declaration exists in the `release`
|
||||
* job's block. It cannot assert that a release actually publishes — that needs a tag push, which
|
||||
* is the thing no PR can do. So this is a tripwire against silent removal, not proof the release
|
||||
* path works.
|
||||
*/
|
||||
class ReleasePermissionTest {
|
||||
|
||||
@Test
|
||||
fun `the release job declares the write permission it needs to publish`() {
|
||||
val release = jobBlock("release")
|
||||
assertTrue(
|
||||
"build.yml's `release` job no longer declares `contents: write`. It is the only " +
|
||||
"permission that lets the job create a release, the top-level block above it is " +
|
||||
"`contents: read`, and nothing else in CI would catch this until a tag failed to " +
|
||||
"publish. If the release moved elsewhere, delete this test deliberately.",
|
||||
release.any { it.trimStart().startsWith("contents: write") },
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* The lines of one top-level job, from its ` <name>:` header to the next job at that indent.
|
||||
*
|
||||
* Line-based rather than parsed: the module has no YAML dependency, and adding one to read two
|
||||
* lines would be a worse trade than a scan that fails loudly when the shape changes.
|
||||
*/
|
||||
private fun jobBlock(name: String): List<String> {
|
||||
val lines = workflow.readLines()
|
||||
val start = lines.indexOfFirst { it == " $name:" }
|
||||
check(start >= 0) { "no ` $name:` job in ${workflow.path} — has the file been restructured?" }
|
||||
val rest = lines.drop(start + 1)
|
||||
val end = rest.indexOfFirst { it.matches(Regex("^ {2}[A-Za-z0-9_-]+:.*")) }
|
||||
return if (end < 0) rest else rest.take(end)
|
||||
}
|
||||
|
||||
/**
|
||||
* Found by walking up rather than by a fixed relative path: Gradle's working directory for the
|
||||
* unit tests is the module, but that is a default rather than a promise.
|
||||
*/
|
||||
private val workflow: File
|
||||
get() = generateSequence(File(".").absoluteFile) { it.parentFile }
|
||||
.map { File(it, ".github/workflows/build.yml") }
|
||||
.firstOrNull { it.isFile }
|
||||
?: error("could not find .github/workflows/build.yml above ${File(".").absolutePath}")
|
||||
}
|
||||
@@ -93,8 +93,12 @@ class ConversionViewModelCleanupTest {
|
||||
assertTrue("a failed save must not destroy the only copy", staged.exists())
|
||||
assertEquals(emptyList<File>(), publisher.discarded)
|
||||
|
||||
// Failed carries no file reference at all, so this only works because the handle is
|
||||
// a ViewModel field rather than something read back out of the state machine.
|
||||
// The handle is a ViewModel field rather than something read back out of the state
|
||||
// machine, and stays one now that a save-failed `Failed` also carries a `PendingSave`:
|
||||
// that is a view for the screen to offer a retry through, never a second owner of the
|
||||
// file. This delete goes through the field, which is what keeps a state that is dropped
|
||||
// rather than read from taking the only reference with it. What the state carries, and
|
||||
// what the screen then does with it, are `FailedSaveRetryTest`'s.
|
||||
viewModel.reset()
|
||||
|
||||
assertEquals(listOf(staged), publisher.discarded)
|
||||
|
||||
@@ -53,10 +53,11 @@ import java.io.File
|
||||
* it -- so it is unobservable from a JVM test, the same limit `FileCardTest` records for
|
||||
* `HorizontalDivider`. The message text itself is asserted; the colour would need a screenshot.
|
||||
* - **The three `assertDoesNotExist` checks on [TestTags.Converter.FILE_CARD] are compile-guarded,
|
||||
* not guarded by this file.** `Idle` is a `data object`, and `Saved` and `Failed` carry only a
|
||||
* `displayName` and a `message`; none of the three has an `input`, so `FileCard(s.input)` does not
|
||||
* compile in those arms. The lines stay because they state the intent cheaply, but they are not
|
||||
* what stops a `FileCard` appearing there and this file does not claim they are.
|
||||
* not guarded by this file.** `Idle` is a `data object`, `Saved` carries a `displayName`, and
|
||||
* `Failed` carries a message and -- after a failed save only -- the staged file it left behind;
|
||||
* none of the three has an `input`, so `FileCard(s.input)` does not compile in those arms. The
|
||||
* lines stay because they state the intent cheaply, but they are not what stops a `FileCard`
|
||||
* appearing there and this file does not claim they are.
|
||||
* - **Which constant each chip hands back** belongs to `ConverterPickerSelectionTest`, and **what
|
||||
* the file card says about an unknown size** to `FileCardTest`. This file asserts that `Ready`
|
||||
* puts those leaves on screen at all, not what they then do.
|
||||
@@ -326,6 +327,22 @@ class ConverterStateAffordancesTest {
|
||||
composeRule.onNodeWithTag(TestTags.Converter.FILE_CARD).assertDoesNotExist()
|
||||
}
|
||||
|
||||
/**
|
||||
* **The assertion that bounds #30's whole change**, and the one worth breaking things to keep.
|
||||
*
|
||||
* A transcode that died staged nothing, so its `Failed` carries no [PendingSave] and there is
|
||||
* nothing for a save dialog to be handed. Making the retry unconditional -- or making the
|
||||
* `WorkInfo.State.FAILED` arm of `ConversionViewModel.observe` carry a handle it has no file
|
||||
* for -- puts a button on screen that can only fail, and this is what notices.
|
||||
*/
|
||||
@Test
|
||||
fun `a transcode failure offers no way to save`() {
|
||||
setContent(ConversionState.Failed(message = "Ran out of space while writing the output."))
|
||||
|
||||
composeRule.onNodeWithTag(TestTags.RETRY_SAVE).assertDoesNotExist()
|
||||
composeRule.onNodeWithTag(TestTags.SAVE_FILE).assertDoesNotExist()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `tapping start over after a failure resets and does nothing else`() {
|
||||
setContent(ConversionState.Failed(message = "Ran out of space while writing the output."))
|
||||
@@ -335,6 +352,50 @@ class ConverterStateAffordancesTest {
|
||||
assertEquals(listOf("reset"), fired)
|
||||
}
|
||||
|
||||
// ----------------------------------------------------- Failed, carrying a file
|
||||
|
||||
/**
|
||||
* The defect in #30, stated as what the branch must render.
|
||||
*
|
||||
* `save()` keeps the staged file on a failure deliberately -- it can be the only copy of an
|
||||
* hour of transcoding -- and before this the only control here was "Start over", wired to
|
||||
* `reset()`, which deletes exactly that file. Both buttons, not one: the restart has to stay
|
||||
* reachable, because leaving a full-size file in cache is the outcome it exists to avoid.
|
||||
*/
|
||||
@Test
|
||||
fun `a failed save offers the file again as well as a restart`() {
|
||||
setContent(failedSave())
|
||||
|
||||
composeRule.onNodeWithTag(TestTags.RETRY_SAVE).assertExists()
|
||||
composeRule.onNodeWithTag(TestTags.START_OVER).assertExists()
|
||||
}
|
||||
|
||||
/**
|
||||
* The name, not just that something fired: it comes from the finished job, and a retry wired to
|
||||
* a literal or to the picker's current guess would hand the dialog a name the job never chose.
|
||||
*/
|
||||
@Test
|
||||
fun `tapping try saving again hands back the name the job chose`() {
|
||||
setContent(failedSave())
|
||||
|
||||
composeRule.onNodeWithTag(TestTags.RETRY_SAVE).performScrollTo().performClick()
|
||||
|
||||
assertEquals(listOf("save:holiday.mp4"), fired)
|
||||
}
|
||||
|
||||
/**
|
||||
* Start over from here still resets, and resetting still deletes -- see `reset()`'s KDoc for
|
||||
* why that is acceptable now and was not before. What it must not do is save on the way past.
|
||||
*/
|
||||
@Test
|
||||
fun `tapping start over after a failed save resets and does not save`() {
|
||||
setContent(failedSave())
|
||||
|
||||
composeRule.onNodeWithTag(TestTags.START_OVER).performScrollTo().performClick()
|
||||
|
||||
assertEquals(listOf("reset"), fired)
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------ Harness
|
||||
|
||||
private fun input() = InputFile(
|
||||
@@ -348,6 +409,21 @@ class ConverterStateAffordancesTest {
|
||||
* missing file rather than throwing, so the size line reads `0 B` and no temporary folder is
|
||||
* needed to render the arm.
|
||||
*/
|
||||
/**
|
||||
* A `Failed` an earlier save left carrying its file, which is the only way [retry] is non-null.
|
||||
*
|
||||
* The same missing `staged` path as [converted], and for the same reason: this arm renders no
|
||||
* size line at all, so nothing here ever touches the filesystem.
|
||||
*/
|
||||
private fun failedSave() = ConversionState.Failed(
|
||||
message = "There was not enough room on the destination.",
|
||||
retry = PendingSave(
|
||||
staged = File("no-such-staged-output.mp4"),
|
||||
suggestedName = "holiday.mp4",
|
||||
mimeType = "video/mp4",
|
||||
),
|
||||
)
|
||||
|
||||
private fun converted(routeReason: String = "") = ConversionState.Converted(
|
||||
input = input(),
|
||||
staged = File("no-such-staged-output.mp4"),
|
||||
|
||||
@@ -0,0 +1,370 @@
|
||||
package org.libremediaconverter.convert
|
||||
|
||||
import android.app.Application
|
||||
import android.net.Uri
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.work.workDataOf
|
||||
import kotlinx.coroutines.Dispatchers
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertNotNull
|
||||
import org.junit.Assert.assertNull
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.join.JoinState
|
||||
import org.libremediaconverter.join.JoinViewModel
|
||||
import org.libremediaconverter.join.pendingSave
|
||||
import org.libremediaconverter.model.InputProbe
|
||||
import org.libremediaconverter.model.OutputFormat
|
||||
import org.libremediaconverter.work.ConcatWorker
|
||||
import org.libremediaconverter.work.ConversionWorker
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import java.io.File
|
||||
|
||||
/**
|
||||
* A failed save has to leave the file *offerable*, not merely undeleted.
|
||||
*
|
||||
* The defect is #30, and both halves of it were already written down in `main`. `save()`'s
|
||||
* `onFailure` kept the staged file on purpose -- "deleting here would destroy the work to tidy up
|
||||
* a cache directory" -- and then handed the screen a `Failed` carrying a message and nothing else,
|
||||
* so the single control that branch rendered was "Start over", wired to `reset()`, which deletes
|
||||
* exactly that file. The intent and the affordance disagreed, and the affordance won.
|
||||
*
|
||||
* `ConversionViewModelCleanupTest` already pins the *keeping*: after a failed save the file is
|
||||
* still on disk and nothing has been discarded. It stays green with the state carrying nothing,
|
||||
* because it reads the filesystem rather than the state. This file asserts the other half -- that
|
||||
* the handle reaches the state a screen can read -- and the negative that bounds it: a failure
|
||||
* with nothing staged behind it must not sprout a save button.
|
||||
*
|
||||
* Both ViewModels in one class, following `MissingStagedFileTest`. They are separate state
|
||||
* machines that can each hold a staged file at once, but this defect and its fix are the same
|
||||
* shape in both, and splitting them would put the two halves of one invariant in two files.
|
||||
*
|
||||
* ### Not asserted here, so each is a decision rather than an omission
|
||||
*
|
||||
* - **That the destination received the bytes.** [RecordingPublisher.publish] is a stub, which is
|
||||
* the only way to make a save fail deterministically -- and making it fail is what every case
|
||||
* here needs. `OutputPublisherPublishTest` owns what a real publish writes.
|
||||
* - **The screen's two buttons.** `ConverterStateAffordancesTest` and `JoinStateAffordancesTest`
|
||||
* own what each state renders; this file owns what each state carries.
|
||||
* - **`ConverterScreen`'s `destinationMime` line itself.** It lives in the entry point, above the
|
||||
* `ScreenContent` seam, and reaching it needs a real ViewModel inside a composition. What it
|
||||
* reads -- `pendingSave()?.mimeType` -- is asserted directly instead, which is why that
|
||||
* derivation was moved out of the entry point in the first place.
|
||||
* - **Picking a new input while a `Failed` carries a file.** `onInputPicked` overwrites the state
|
||||
* without discarding, from `Converted` exactly as much as from a carrying `Failed`, and neither
|
||||
* branch renders a picker. It is a pre-existing path this change neither opens nor widens: the
|
||||
* carried handle is a view of `pendingStaged`, never a second owner of the file.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class FailedSaveRetryTest {
|
||||
|
||||
private lateinit var app: Application
|
||||
private lateinit var publisher: RecordingPublisher
|
||||
private lateinit var staged: File
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
app = RuntimeEnvironment.getApplication()
|
||||
publisher = RecordingPublisher(app)
|
||||
ConversionDependencies.publisher = { publisher }
|
||||
// MediaProbe spawns FFprobe, whose loader throws with no native library present.
|
||||
ConversionDependencies.probe = { _, _ -> InputProbe() }
|
||||
|
||||
staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(ByteArray(4096)) }
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() {
|
||||
ConversionDependencies.reset()
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------ Convert
|
||||
|
||||
/**
|
||||
* The bite named in #30's fix. Emitting a plain `Failed` from `save()`'s `onFailure` -- which
|
||||
* is what `main` did -- reddens this case on the null handle, and nothing else in the suite.
|
||||
*/
|
||||
@Test
|
||||
fun `a failed save leaves the staged file offerable, not merely undeleted`() {
|
||||
val viewModel = failedSaveViewModel()
|
||||
|
||||
val failed = viewModel.state.value as ConversionState.Failed
|
||||
val retry = failed.retry
|
||||
assertNotNull("a failed save must leave the staged file offerable, not just on disk", retry)
|
||||
assertEquals("the retry must name the file the conversion actually produced", staged, retry?.staged)
|
||||
// Both from the job's own output Data rather than from the pickers, so a retry opens the
|
||||
// same dialog the first attempt did.
|
||||
assertEquals(SUGGESTED_NAME, retry?.suggestedName)
|
||||
assertEquals(JOB_MIME_TYPE, retry?.mimeType)
|
||||
assertTrue("a failed save must not destroy the only copy", staged.exists())
|
||||
}
|
||||
|
||||
/**
|
||||
* The whole point of carrying the handle: the second attempt is a real save, not a new job.
|
||||
*
|
||||
* `publishFailure` is cleared between the two calls, so one `RecordingPublisher` plays both a
|
||||
* full destination and an empty one -- which is exactly the user's situation.
|
||||
*/
|
||||
@Test
|
||||
fun `retrying a failed save publishes the file and leaves nothing staged`() {
|
||||
val viewModel = failedSaveViewModel()
|
||||
|
||||
publisher.publishFailure = null
|
||||
viewModel.save(DESTINATION)
|
||||
|
||||
val saved = awaitState(viewModel.state, "Saved") { it is ConversionState.Saved }
|
||||
assertEquals(SUGGESTED_NAME, (saved as ConversionState.Saved).displayName)
|
||||
assertFalse("a successful retry should have removed the staged file", staged.exists())
|
||||
// Nothing to collect afterwards: the retry published it, so reset() has no work left.
|
||||
viewModel.reset()
|
||||
assertEquals(emptyList<File>(), publisher.discarded)
|
||||
}
|
||||
|
||||
/**
|
||||
* The second failure must not eat the file the first one kept.
|
||||
*
|
||||
* A `Failed` built fresh from `e.message` alone would drop the handle here while every other
|
||||
* assertion in this file stayed green -- the file is still on disk, and the first failure
|
||||
* already proved the state can carry it.
|
||||
*/
|
||||
@Test
|
||||
fun `a retry that fails again still carries the file rather than dropping it`() {
|
||||
val viewModel = failedSaveViewModel()
|
||||
|
||||
publisher.publishFailure = IllegalStateException("destination volume still full")
|
||||
viewModel.save(DESTINATION)
|
||||
|
||||
// Waited for by the *second* message rather than by `is Failed`: the state was already
|
||||
// Failed when the retry started, so the type alone would be satisfied before it ran.
|
||||
val failed = awaitState(viewModel.state, "the second failure") {
|
||||
it is ConversionState.Failed && it.message == "destination volume still full"
|
||||
} as ConversionState.Failed
|
||||
assertEquals("the second failure must offer the same file the first one did", staged, failed.retry?.staged)
|
||||
assertTrue(staged.exists())
|
||||
assertEquals(emptyList<File>(), publisher.discarded)
|
||||
}
|
||||
|
||||
/**
|
||||
* "Start over" still deletes, and that is the decision `reset()`'s KDoc records: acceptable
|
||||
* only because "Try saving again" is on screen beside it. Exactly once, through the publisher.
|
||||
*/
|
||||
@Test
|
||||
fun `start over from a failed save discards the carried file exactly once`() {
|
||||
val viewModel = failedSaveViewModel()
|
||||
|
||||
viewModel.reset()
|
||||
|
||||
assertEquals(ConversionState.Idle, viewModel.state.value)
|
||||
assertEquals(listOf(staged), publisher.discarded)
|
||||
assertFalse(staged.exists())
|
||||
}
|
||||
|
||||
/**
|
||||
* A retry meets the same existence check the first attempt did, so a file collected by the
|
||||
* sweep or by the OS in between is reported as a sentence rather than as a raw ENOENT path.
|
||||
* And the state that reports it carries nothing: there is no file left to offer.
|
||||
*/
|
||||
@Test
|
||||
fun `a retry whose staged file has gone says so and offers nothing further`() {
|
||||
val viewModel = failedSaveViewModel()
|
||||
assertTrue("the fixture must start with a real staged file", staged.delete())
|
||||
|
||||
publisher.publishFailure = null
|
||||
viewModel.save(DESTINATION)
|
||||
|
||||
val failed = viewModel.state.value as ConversionState.Failed
|
||||
assertEquals(STAGED_FILE_GONE_MESSAGE, failed.message)
|
||||
assertNull("a file that has gone cannot be offered again", failed.retry)
|
||||
}
|
||||
|
||||
/**
|
||||
* The negative that bounds the whole change, and the reason `retry` is nullable.
|
||||
*
|
||||
* A transcode that died staged nothing, so there is no file to hand back -- and a `Failed`
|
||||
* that carried one anyway would put a save button on a screen with nothing to save. Driven
|
||||
* through a worker that really fails rather than by constructing the state, because the line
|
||||
* under test is the `WorkInfo.State.FAILED` arm of `observe`.
|
||||
*/
|
||||
@Test
|
||||
fun `a transcode failure carries nothing to save`() {
|
||||
installFailingTestWorkManager(app, workDataOf(ConversionWorker.KEY_ERROR to "The encoder gave up."))
|
||||
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
|
||||
viewModel.onInputPicked(Uri.parse("content://test/holiday.mp4"))
|
||||
awaitState(viewModel.state, "Ready") { it is ConversionState.Ready }
|
||||
|
||||
viewModel.convert()
|
||||
|
||||
val failed = awaitState(viewModel.state, "Failed") { it is ConversionState.Failed } as ConversionState.Failed
|
||||
assertEquals("The encoder gave up.", failed.message)
|
||||
assertNull("a transcode failure has nothing staged, so it must offer no save", failed.retry)
|
||||
assertNull("and nothing for the save dialog to open with either", failed.pendingSave())
|
||||
}
|
||||
|
||||
/**
|
||||
* What the save dialog reopens with, which is the entry point's only reader of this state.
|
||||
*
|
||||
* The pickers are moved *after* the job finishes, which is what makes this bite: a retry that
|
||||
* asked the current settings would offer `audio/mpeg` for a file the job wrote as MP4. The
|
||||
* same gap is permanent for a reattached job, whose spec was never in these settings at all.
|
||||
*/
|
||||
@Test
|
||||
fun `a retry offers the type the job chose, not the one the pickers now show`() {
|
||||
val viewModel = failedSaveViewModel()
|
||||
|
||||
viewModel.setPreset(OutputFormat.MP3)
|
||||
|
||||
assertEquals(
|
||||
"the fixture needs the pickers to disagree with the job",
|
||||
"audio/mpeg",
|
||||
viewModel.settings.value.spec.mimeType,
|
||||
)
|
||||
assertEquals(JOB_MIME_TYPE, viewModel.state.value.pendingSave()?.mimeType)
|
||||
}
|
||||
|
||||
// --------------------------------------------------------------------- Join
|
||||
|
||||
@Test
|
||||
fun `a failed join save leaves the staged file offerable, not merely undeleted`() {
|
||||
val viewModel = failedJoinSaveViewModel()
|
||||
|
||||
val failed = viewModel.state.value as JoinState.Failed
|
||||
val retry = failed.retry
|
||||
assertNotNull("a failed save must leave the staged file offerable, not just on disk", retry)
|
||||
assertEquals(staged, retry?.staged)
|
||||
assertEquals(SUGGESTED_NAME, retry?.suggestedName)
|
||||
assertEquals(JOB_MIME_TYPE, retry?.mimeType)
|
||||
assertTrue(staged.exists())
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `retrying a failed join save publishes the file and leaves nothing staged`() {
|
||||
val viewModel = failedJoinSaveViewModel()
|
||||
|
||||
publisher.publishFailure = null
|
||||
viewModel.save(DESTINATION)
|
||||
|
||||
val saved = awaitState(viewModel.state, "Saved") { it is JoinState.Saved }
|
||||
assertEquals(SUGGESTED_NAME, (saved as JoinState.Saved).displayName)
|
||||
assertFalse(staged.exists())
|
||||
viewModel.reset()
|
||||
assertEquals(emptyList<File>(), publisher.discarded)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a join retry that fails again still carries the file rather than dropping it`() {
|
||||
val viewModel = failedJoinSaveViewModel()
|
||||
|
||||
publisher.publishFailure = IllegalStateException("destination volume still full")
|
||||
viewModel.save(DESTINATION)
|
||||
|
||||
// By the second message, not by `is Failed` -- see the converter case above.
|
||||
val failed = awaitState(viewModel.state, "the second failure") {
|
||||
it is JoinState.Failed && it.message == "destination volume still full"
|
||||
} as JoinState.Failed
|
||||
assertEquals(staged, failed.retry?.staged)
|
||||
assertTrue(staged.exists())
|
||||
assertEquals(emptyList<File>(), publisher.discarded)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `start over from a failed join save discards the carried file exactly once`() {
|
||||
val viewModel = failedJoinSaveViewModel()
|
||||
|
||||
viewModel.reset()
|
||||
|
||||
assertEquals(JoinState.Idle, viewModel.state.value)
|
||||
assertEquals(listOf(staged), publisher.discarded)
|
||||
assertFalse(staged.exists())
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a join failure carries nothing to save`() {
|
||||
installFailingTestWorkManager(app, workDataOf(ConcatWorker.KEY_ERROR to "The files could not be joined."))
|
||||
val viewModel = JoinViewModel(app, Dispatchers.Unconfined)
|
||||
viewModel.onInputsPicked(listOf(Uri.parse("content://test/a.mp4"), Uri.parse("content://test/b.mp4")))
|
||||
awaitState(viewModel.state, "Ready") { it is JoinState.Ready }
|
||||
|
||||
viewModel.join()
|
||||
|
||||
val failed = awaitState(viewModel.state, "Failed") { it is JoinState.Failed } as JoinState.Failed
|
||||
assertEquals("The files could not be joined.", failed.message)
|
||||
assertNull("a join failure has nothing staged, so it must offer no save", failed.retry)
|
||||
assertNull(failed.pendingSave())
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------ Harness
|
||||
|
||||
/**
|
||||
* A ViewModel driven to `Converted` and then through a save that threw.
|
||||
*
|
||||
* The WorkManager is installed here rather than in `@Before`, because two cases in this class
|
||||
* need one whose workers fail instead.
|
||||
*/
|
||||
private fun failedSaveViewModel(): ConversionViewModel {
|
||||
installTestWorkManager(app, conversionOutput())
|
||||
// Unconfined so reset()'s delete runs inline instead of on a real IO thread.
|
||||
val viewModel = ConversionViewModel(app, Dispatchers.Unconfined)
|
||||
viewModel.onInputPicked(Uri.parse("content://test/holiday.mkv"))
|
||||
awaitState(viewModel.state, "Ready") { it is ConversionState.Ready }
|
||||
viewModel.convert()
|
||||
awaitState(viewModel.state, "Converted") { it is ConversionState.Converted }
|
||||
|
||||
publisher.publishFailure = IllegalStateException("destination volume full")
|
||||
viewModel.save(DESTINATION)
|
||||
awaitState(viewModel.state, "Failed") { it is ConversionState.Failed }
|
||||
return viewModel
|
||||
}
|
||||
|
||||
/** The join tab's equivalent, driven to `Joined` and then through a save that threw. */
|
||||
private fun failedJoinSaveViewModel(): JoinViewModel {
|
||||
installTestWorkManager(app, joinOutput())
|
||||
val viewModel = JoinViewModel(app, Dispatchers.Unconfined)
|
||||
viewModel.onInputsPicked(listOf(Uri.parse("content://test/a.mp4"), Uri.parse("content://test/b.mp4")))
|
||||
awaitState(viewModel.state, "Ready") { it is JoinState.Ready }
|
||||
viewModel.join()
|
||||
awaitState(viewModel.state, "Joined") { it is JoinState.Joined }
|
||||
|
||||
publisher.publishFailure = IllegalStateException("destination volume full")
|
||||
viewModel.save(DESTINATION)
|
||||
awaitState(viewModel.state, "Failed") { it is JoinState.Failed }
|
||||
return viewModel
|
||||
}
|
||||
|
||||
/**
|
||||
* The output `Data` a finished conversion reports.
|
||||
*
|
||||
* The name and type are set rather than left out, so the assertions above are about what the
|
||||
* *job* chose. Both ViewModels fall back to a derivation when they are missing, and a fixture
|
||||
* that omitted them would be asserting the fallback while looking like it asserted the job.
|
||||
*
|
||||
* Spelled out per worker rather than shared with [joinOutput], even though the two constants
|
||||
* hold the same strings today. A test that leaned on that would be asserting a coincidence.
|
||||
*/
|
||||
private fun conversionOutput() = workDataOf(
|
||||
ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath,
|
||||
ConversionWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME,
|
||||
ConversionWorker.KEY_MIME_TYPE to JOB_MIME_TYPE,
|
||||
)
|
||||
|
||||
/** The output `Data` a finished join reports. See [conversionOutput]. */
|
||||
private fun joinOutput() = workDataOf(
|
||||
ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath,
|
||||
ConcatWorker.KEY_SUGGESTED_NAME to SUGGESTED_NAME,
|
||||
ConcatWorker.KEY_MIME_TYPE to JOB_MIME_TYPE,
|
||||
)
|
||||
|
||||
private companion object {
|
||||
val DESTINATION: Uri = Uri.parse("content://test/destination.mp4")
|
||||
|
||||
const val SUGGESTED_NAME = "holiday.mp4"
|
||||
|
||||
/** What the job wrote. [OutputFormat.MP3]'s `audio/mpeg` is what the pickers move to. */
|
||||
const val JOB_MIME_TYPE = "video/mp4"
|
||||
}
|
||||
}
|
||||
+105
@@ -0,0 +1,105 @@
|
||||
package org.libremediaconverter.convert
|
||||
|
||||
import android.net.Uri
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import kotlinx.coroutines.runBlocking
|
||||
import kotlinx.coroutines.withTimeout
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.model.AudioCodec
|
||||
import org.libremediaconverter.model.AudioPlan
|
||||
import org.libremediaconverter.model.Container
|
||||
import org.libremediaconverter.model.ConversionRequest
|
||||
import org.libremediaconverter.model.CopyPlanner
|
||||
import org.libremediaconverter.model.InputKind
|
||||
import org.libremediaconverter.model.InputProbe
|
||||
import org.libremediaconverter.model.OutputSpec
|
||||
import org.libremediaconverter.model.VideoCodec
|
||||
import org.libremediaconverter.model.VideoPlan
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import java.io.File
|
||||
import java.util.concurrent.CancellationException
|
||||
|
||||
/**
|
||||
* What happens when Media3 refuses the export before it starts.
|
||||
*
|
||||
* `EditedMediaItem.Builder` rejects a composition with both tracks removed —
|
||||
* checkState("Audio and video cannot both be removed") — and [Media3Engine] builds it on its own
|
||||
* HandlerThread. That build used to sit *between* two narrow `runCatching` blocks, one around
|
||||
* `buildTransformer` and one around `start`, so the exception escaped `handler.post`'s body: it
|
||||
* reached the thread's uncaught handler, which on Android takes the process down, and the
|
||||
* continuation was left unresumed either way.
|
||||
*
|
||||
* Robolectric runs the real [android.os.HandlerThread] and the real Media3 builders, so the whole
|
||||
* sequence happens here — the engine really posts, really builds, and really throws. What it cannot
|
||||
* reproduce is the *consequence* of an escaped throw: a JVM background thread dying is not process
|
||||
* death. So the assertion is on the half that is observable everywhere and is the half that
|
||||
* matters to the user — the suspension is resolved, with the reason, rather than left hanging.
|
||||
* `Media3EngineTest.aPlanThatRemovesBothTracksFailsInsteadOfKillingTheProcess` is the same case on
|
||||
* a device.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class Media3EngineEmptyCompositionTest {
|
||||
|
||||
@Test
|
||||
fun `a plan that removes both tracks fails the job instead of escaping the handler thread`() {
|
||||
val context = RuntimeEnvironment.getApplication()
|
||||
val engine = Media3Engine(context)
|
||||
val request = ConversionRequest(
|
||||
spec = OutputSpec(Container.MP4, VideoCodec.H265, AudioCodec.NONE),
|
||||
probe = InputProbe(
|
||||
videoCodec = null,
|
||||
audioCodec = "mp3",
|
||||
hasVideo = false,
|
||||
container = Container.MP3,
|
||||
kind = InputKind.AUDIO_ONLY,
|
||||
),
|
||||
)
|
||||
|
||||
// Asserted rather than assumed: ConversionRequest's default probe says hasVideo = true, and
|
||||
// with it this same spec plans to (Encode, Drop), nothing throws, and the test would pass
|
||||
// over a code path it never entered.
|
||||
val plan = CopyPlanner.plan(request.spec, request.probe)
|
||||
assertEquals(VideoPlan.Drop, plan.video)
|
||||
assertEquals(AudioPlan.Drop, plan.audio)
|
||||
|
||||
val failure = try {
|
||||
runCatching {
|
||||
runBlocking {
|
||||
withTimeout(TIMEOUT_MS) {
|
||||
engine.transcode(Uri.parse("file:///dev/null"), File(context.cacheDir, "empty.mp4"), request) {}
|
||||
}
|
||||
}
|
||||
}.exceptionOrNull()
|
||||
} finally {
|
||||
engine.close()
|
||||
}
|
||||
|
||||
// Both halves are load-bearing, and the second is not pedantry: withTimeout raises
|
||||
// TimeoutCancellationException, and `java.util.concurrent.CancellationException` *extends*
|
||||
// IllegalStateException — so testing only the first would call an unresumed continuation a
|
||||
// pass. This assertion was written that way, and the mutation is what found it.
|
||||
assertFalse(
|
||||
"the continuation was never resumed — the failure escaped instead of being reported: $failure",
|
||||
failure is CancellationException,
|
||||
)
|
||||
assertTrue(
|
||||
"the builder's refusal must surface as a failed job; got $failure",
|
||||
failure is IllegalStateException,
|
||||
)
|
||||
}
|
||||
|
||||
private companion object {
|
||||
/**
|
||||
* Short on purpose. Nothing is decoded, encoded or muxed on this path — the builder refuses
|
||||
* the input outright — so anything approaching this is a hang, which is the failure mode
|
||||
* this test is looking for.
|
||||
*/
|
||||
const val TIMEOUT_MS = 10_000L
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,63 @@
|
||||
package org.libremediaconverter.convert
|
||||
|
||||
import kotlinx.coroutines.CoroutineDispatcher
|
||||
import java.util.concurrent.ConcurrentLinkedQueue
|
||||
import kotlin.coroutines.CoroutineContext
|
||||
|
||||
/**
|
||||
* A dispatcher that holds a pick in flight until the test lets it finish.
|
||||
*
|
||||
* Issue #49 is about what a ViewModel does *while* a pick is between the tap and the write it
|
||||
* eventually makes. Both ViewModels put a blocking hop there — the metadata query, and on the
|
||||
* convert side the probe as well — and both hops go through an injectable dispatcher. Handing
|
||||
* them this one turns "the pick has been made but has not landed yet" from a window a test has
|
||||
* to race into a state it can simply sit in.
|
||||
*
|
||||
* Nothing here is a fake pick. The real `InputQuery.describe` still runs, on this thread,
|
||||
* whenever [runAll] is called; the only thing under the test's control is *when*.
|
||||
*
|
||||
* Confined to the thread that drives the test. Both ViewModels reach `withContext(pickDispatcher)`
|
||||
* from a coroutine on the main dispatcher, so [dispatch] is only ever called from there — the
|
||||
* queue is concurrent anyway, because a dispatcher that quietly dropped a block from another
|
||||
* thread would fail as a hang rather than as an assertion.
|
||||
*/
|
||||
class ParkedPickDispatcher : CoroutineDispatcher() {
|
||||
|
||||
private val parked = ConcurrentLinkedQueue<Runnable>()
|
||||
|
||||
/**
|
||||
* How many blocks are waiting.
|
||||
*
|
||||
* Asserted on before the interesting part of a test, because "the pick was in flight" is a
|
||||
* premise rather than a detail: a zero here means the pick had already landed and whatever
|
||||
* the test went on to prove was proved about a different situation.
|
||||
*/
|
||||
val parkedCount: Int get() = parked.size
|
||||
|
||||
override fun dispatch(context: CoroutineContext, block: Runnable) {
|
||||
parked += block
|
||||
}
|
||||
|
||||
/**
|
||||
* Removes everything parked, oldest first, and hands it to the caller to run.
|
||||
*
|
||||
* What [runAll] cannot express: two picks are two hops through this dispatcher, and the defect
|
||||
* they can produce is the *first* one finishing last. Running them in the order they arrived
|
||||
* is the one order in which nothing goes wrong, so a test has to be able to choose.
|
||||
*/
|
||||
fun takeParked(): List<Runnable> = generateSequence { parked.poll() }.toList()
|
||||
|
||||
/**
|
||||
* Runs everything parked, and everything that parks as a result.
|
||||
*
|
||||
* The loop is not defensive: `ConversionViewModel.onInputPicked` makes two hops through this
|
||||
* dispatcher — the metadata query, then the probe — and the second is only enqueued once the
|
||||
* first has run. Draining once would leave the probe parked for the rest of the process.
|
||||
*/
|
||||
fun runAll() {
|
||||
while (true) {
|
||||
val next = parked.poll() ?: return
|
||||
next.run()
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,125 @@
|
||||
package org.libremediaconverter.convert
|
||||
|
||||
import android.app.Application
|
||||
import android.net.Uri
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.work.workDataOf
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertNotNull
|
||||
import org.junit.Assert.assertNull
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.model.InputProbe
|
||||
import org.libremediaconverter.work.ConversionWorker
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import java.io.File
|
||||
|
||||
/**
|
||||
* The half of issue #49 that is not about reattachment at all.
|
||||
*
|
||||
* `onInputPicked` makes two writes and both of them land after a hop off the main thread, so both
|
||||
* belong to whichever pick was in flight rather than to whichever pick the user last made. Nothing
|
||||
* was enforcing that. Two taps in quick succession — an easy thing to do while a `content://`
|
||||
* metadata query is slow — put the loser's file on screen if its query happened to come back
|
||||
* second, which is the same defect the ticket reported against reattachment with a different
|
||||
* coroutine on the losing side.
|
||||
*
|
||||
* Both cases below were measured rather than assumed: deleting either guard turns the matching
|
||||
* test red, and deleting the probe one turns nine other tests red with it. Neither was ever
|
||||
* reported, because a pick that loses to another pick still shows *a* file the user chose -- which
|
||||
* is what made it worth closing alongside #49 rather than leaving as a second thing to find.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class PickOwnershipTest {
|
||||
|
||||
private lateinit var app: Application
|
||||
private lateinit var parkedPick: ParkedPickDispatcher
|
||||
private lateinit var viewModel: ConversionViewModel
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
app = RuntimeEnvironment.getApplication()
|
||||
ConversionDependencies.publisher = { RecordingPublisher(app) }
|
||||
ConversionDependencies.probe = { _, _ -> PROBE }
|
||||
installTestWorkManager(app, workDataOf(ConversionWorker.KEY_OUTPUT_PATH to "/dev/null"))
|
||||
|
||||
parkedPick = ParkedPickDispatcher()
|
||||
viewModel = ConversionViewModel(app, pickDispatcher = parkedPick)
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() {
|
||||
ConversionDependencies.reset()
|
||||
}
|
||||
|
||||
/**
|
||||
* Two taps, and the first one's metadata query is the slow one.
|
||||
*
|
||||
* The order is chosen rather than raced: both queries are parked, and this runs the second
|
||||
* before the first. Without an ownership check the straggler writes last and the screen ends
|
||||
* up showing a file the user moved off two taps ago.
|
||||
*/
|
||||
@Test
|
||||
fun `the slower of two picks does not land on top of the faster one`() {
|
||||
viewModel.onInputPicked(FIRST)
|
||||
viewModel.onInputPicked(SECOND)
|
||||
|
||||
val queries = parkedPick.takeParked()
|
||||
assertEquals("both picks should be in flight", 2, queries.size)
|
||||
// The second pick's query comes back first; the first pick's is the straggler.
|
||||
queries[1].run()
|
||||
queries[0].run()
|
||||
|
||||
val current = viewModel.state.value
|
||||
assertEquals(
|
||||
"a pick the user has already replaced took the screen: $current",
|
||||
SECOND,
|
||||
(current as ConversionState.Ready).input.uri,
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* The second of `onInputPicked`'s two writes, which lands a whole probe later.
|
||||
*
|
||||
* The probe hop is a native process spawn, so it is the longest gap in a pick and the easiest
|
||||
* one to pick again during. This used to be guarded by comparing URIs against the state, which
|
||||
* answers a narrower question than the one that matters — it cannot tell a second pick of the
|
||||
* same file from the first, and it reads a state that a later claim may not have written yet,
|
||||
* which is exactly this case: the newer pick has claimed the screen but its own query has not
|
||||
* come back, so the state still names the older file and the comparison waves it through.
|
||||
*/
|
||||
@Test
|
||||
fun `a probe from a pick the user has moved off does not fill the card in`() {
|
||||
viewModel.onInputPicked(FIRST)
|
||||
parkedPick.takeParked().single().run()
|
||||
assertEquals(FIRST, (viewModel.state.value as ConversionState.Ready).input.uri)
|
||||
|
||||
// The user picks again while the first pick is still probing.
|
||||
viewModel.onInputPicked(SECOND)
|
||||
val pending = parkedPick.takeParked()
|
||||
assertEquals("the first probe and the second query should both be waiting", 2, pending.size)
|
||||
pending[0].run()
|
||||
|
||||
assertNull(
|
||||
"a probe belonging to a pick the user replaced must not reach the card",
|
||||
(viewModel.state.value as ConversionState.Ready).input.probe,
|
||||
)
|
||||
|
||||
// And the pick that did win still fills its own card in, probe included.
|
||||
pending[1].run()
|
||||
parkedPick.runAll()
|
||||
val settled = viewModel.state.value as ConversionState.Ready
|
||||
assertEquals(SECOND, settled.input.uri)
|
||||
assertNotNull("the winning pick's own probe still has to land", settled.input.probe)
|
||||
}
|
||||
|
||||
private companion object {
|
||||
val FIRST: Uri = Uri.fromFile(File("/tmp/first.mp4"))
|
||||
val SECOND: Uri = Uri.fromFile(File("/tmp/second.mp4"))
|
||||
val PROBE = InputProbe(videoCodec = "h264")
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,156 @@
|
||||
package org.libremediaconverter.convert
|
||||
|
||||
import android.app.Application
|
||||
import android.net.Uri
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.work.WorkManager
|
||||
import androidx.work.workDataOf
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.model.InputProbe
|
||||
import org.libremediaconverter.work.ConversionWorker
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import java.io.File
|
||||
|
||||
/**
|
||||
* Issue #49, on the JVM and without the race.
|
||||
*
|
||||
* `ReattachOnLaunchTest.doesNotOverwriteAPickTheUserHasAlreadyMade` has been catching this on
|
||||
* devices since 2026-08-24 — four occurrences, spread across API 33, 35 and 36, which is what
|
||||
* ruled out an emulator-image quirk. Every one was on attempt 1 and every one passed on re-run,
|
||||
* which is why it was read as flaky infrastructure for two days. It is not. The assertion it fails
|
||||
* on is `expected null, but was:<Converted>`: a finished job from an earlier session taking a
|
||||
* screen the user had already picked a file on.
|
||||
*
|
||||
* The defect is a check-then-act whose act is deferred into another coroutine. `reattach()` reads
|
||||
* `_state.value` and then calls `observe()`, which *launches* a collector that has to suspend on
|
||||
* `getWorkInfoByIdFlow(...).collect` before it can write anything. So the check happens at one
|
||||
* moment and the write lands at another:
|
||||
*
|
||||
* 1. `init` starts the tag query and suspends in it.
|
||||
* 2. The user picks a file; `onInputPicked` suspends in its metadata query.
|
||||
* 3. The query comes back. `_state.value` is still `Idle` — step 2 has not written yet — so the
|
||||
* guard passes and an observation of the old job is launched.
|
||||
* 4. The pick lands. `Ready(picked)`. The user owns the screen.
|
||||
* 5. The observation's first `WorkInfo` arrives and writes `Converted(yesterday)` over it.
|
||||
*
|
||||
* The comment above that guard claimed "no suspension point between this check and the assignment
|
||||
* below, so nothing can interleave". There is no assignment below, and the check and the write
|
||||
* are in different coroutines.
|
||||
*
|
||||
* [ReattachGuardsTest] covers the case where the pick has already *landed*, which the plain guard
|
||||
* does catch. This covers the one where it is still in flight, which it does not.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class ReattachmentOwnershipTest {
|
||||
|
||||
private lateinit var app: Application
|
||||
private lateinit var publisher: RecordingPublisher
|
||||
private lateinit var workManager: WorkManager
|
||||
private lateinit var staged: File
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
app = RuntimeEnvironment.getApplication()
|
||||
publisher = RecordingPublisher(app)
|
||||
ConversionDependencies.publisher = { publisher }
|
||||
ConversionDependencies.probe = { _, _ -> InputProbe() }
|
||||
|
||||
staged = publisher.createStagingFile("holiday_converted.mp4").apply { writeBytes(ByteArray(4096)) }
|
||||
installTestWorkManager(app, workDataOf(ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath))
|
||||
workManager = WorkManager.getInstance(app)
|
||||
// The situation reattachment exists for: a conversion that finished in a process that is
|
||||
// gone, with its output still in the cache and nothing in the UI holding its id.
|
||||
workManager.enqueue(
|
||||
ConversionWorker.request(
|
||||
inputUri = Uri.parse("content://test/holiday.mp4"),
|
||||
displayName = "holiday.mp4",
|
||||
sizeBytes = 4_096L,
|
||||
),
|
||||
).result.get()
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() {
|
||||
ConversionDependencies.reset()
|
||||
}
|
||||
|
||||
/**
|
||||
* The race, made into a state the test can sit in rather than one it has to catch.
|
||||
*
|
||||
* The pick is parked on a dispatcher this test owns, so it stays in flight — issued, not yet
|
||||
* written — for as long as the assertions need it to be. Everything else is production: a
|
||||
* real `WorkManager` holding a real finished job, the real `reattach`, the real `observe`.
|
||||
*
|
||||
* Determinism comes from where Robolectric leaves the main looper. `reattach`'s tag query hops
|
||||
* to a real [kotlinx.coroutines.Dispatchers.IO] thread, so its continuation can only come back
|
||||
* as a message posted to the main looper — and that looper is paused, so it cannot run until
|
||||
* something pumps it. `onInputPicked` is an ordinary synchronous call from this thread. The
|
||||
* pick is therefore *always* issued before the guard runs; none of it is left to timing, which
|
||||
* is the whole point of writing it here rather than relying on the rare device sighting.
|
||||
*/
|
||||
@Test
|
||||
fun `a conversion found while the user was picking never reaches the screen`() {
|
||||
val picked = Uri.fromFile(File(app.cacheDir, "beach.mp4").apply { writeBytes(ByteArray(2048)) })
|
||||
val parkedPick = ParkedPickDispatcher()
|
||||
val viewModel = ConversionViewModel(app, pickDispatcher = parkedPick)
|
||||
viewModel.onInputPicked(picked)
|
||||
|
||||
assertEquals(
|
||||
"the pick must still be in flight, or this proves something about a different situation",
|
||||
1,
|
||||
parkedPick.parkedCount,
|
||||
)
|
||||
|
||||
// The control, and the reason this test does not rest on a settle window being long
|
||||
// enough. A second ViewModel with nothing to supersede it reattaches to the same job
|
||||
// through the same code; when it has arrived, the whole query-guard-observe-write path has
|
||||
// demonstrably run to completion. `viewModel` started its own reattachment first, so it
|
||||
// has had at least as long. Waiting on this rather than on a sleep is what makes the
|
||||
// assertion below "it did not happen" rather than "it had not happened yet".
|
||||
reattachmentHasRunToCompletion()
|
||||
|
||||
val current = viewModel.state.value
|
||||
assertTrue("reattachment took the screen from the user: $current", current is ConversionState.Idle)
|
||||
|
||||
// And the pick, when it lands, is what stays there.
|
||||
parkedPick.runAll()
|
||||
val ready = awaitState(viewModel.state, "Ready") { it is ConversionState.Ready }
|
||||
assertEquals(picked, (ready as ConversionState.Ready).input.uri)
|
||||
reattachmentHasRunToCompletion()
|
||||
assertEquals("the user's pick must survive a late reattachment", ready, viewModel.state.value)
|
||||
}
|
||||
|
||||
/**
|
||||
* The other half of the contract: a reattachment nobody has superseded still takes the screen.
|
||||
*
|
||||
* Without this, dropping every reattachment on the floor would pass the test above. Same job,
|
||||
* same WorkManager, same production path — only the pick is missing.
|
||||
*/
|
||||
@Test
|
||||
fun `a conversion nobody has superseded still reaches the screen`() {
|
||||
val converted = awaitState(ConversionViewModel(app).state, "Converted") {
|
||||
it is ConversionState.Converted
|
||||
}
|
||||
|
||||
assertEquals(staged.absolutePath, (converted as ConversionState.Converted).staged.absolutePath)
|
||||
assertEquals("holiday.mp4", converted.input.displayName)
|
||||
}
|
||||
|
||||
/**
|
||||
* Drives a throwaway ViewModel through a whole reattachment, and returns once it has landed.
|
||||
*
|
||||
* [awaitState] pumps the main looper, which is what runs every reattachment continuation
|
||||
* waiting on it — this one's, and the one belonging to the ViewModel under test, which was
|
||||
* posted earlier and therefore runs first.
|
||||
*/
|
||||
private fun reattachmentHasRunToCompletion() {
|
||||
awaitState(ConversionViewModel(app).state, "Converted") { it is ConversionState.Converted }
|
||||
}
|
||||
}
|
||||
@@ -82,6 +82,28 @@ class SucceedingWorkerFactory(private val outputData: Data) : WorkerFactory() {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Stands in for a worker that died, reporting [outputData] on the way out.
|
||||
*
|
||||
* The counterpart to [SucceedingWorkerFactory], and needed for the same reason: the real workers
|
||||
* cannot run on the JVM, so the only way to ask what a ViewModel does with a `FAILED` `WorkInfo` is
|
||||
* to produce one. A `Result.failure` carrying `KEY_ERROR` is exactly what both real workers report
|
||||
* when their engine gives up, and it is the one path where nothing has ever been staged.
|
||||
*
|
||||
* `runAttemptCount` is irrelevant here: `Result.failure` is terminal, so WorkManager does not retry
|
||||
* it and the state goes straight to `Failed` rather than through `Waiting`.
|
||||
*/
|
||||
class FailingWorkerFactory(private val outputData: Data) : WorkerFactory() {
|
||||
|
||||
override fun createWorker(
|
||||
appContext: Context,
|
||||
workerClassName: String,
|
||||
workerParameters: WorkerParameters,
|
||||
): ListenableWorker = object : Worker(appContext, workerParameters) {
|
||||
override fun doWork(): Result = Result.failure(outputData)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Installs a synchronous test WorkManager whose workers succeed with [outputData].
|
||||
*
|
||||
@@ -89,6 +111,16 @@ class SucceedingWorkerFactory(private val outputData: Data) : WorkerFactory() {
|
||||
*/
|
||||
fun installTestWorkManager(context: Context, outputData: Data): SucceedingWorkerFactory {
|
||||
val factory = SucceedingWorkerFactory(outputData)
|
||||
installWorkManager(context, factory)
|
||||
return factory
|
||||
}
|
||||
|
||||
/** Installs a synchronous test WorkManager whose workers fail, reporting [outputData]. */
|
||||
fun installFailingTestWorkManager(context: Context, outputData: Data) {
|
||||
installWorkManager(context, FailingWorkerFactory(outputData))
|
||||
}
|
||||
|
||||
private fun installWorkManager(context: Context, factory: WorkerFactory) {
|
||||
WorkManagerTestInitHelper.initializeTestWorkManager(
|
||||
context,
|
||||
Configuration.Builder()
|
||||
@@ -98,7 +130,6 @@ fun installTestWorkManager(context: Context, outputData: Data): SucceedingWorker
|
||||
.setWorkerFactory(factory)
|
||||
.build(),
|
||||
)
|
||||
return factory
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -0,0 +1,87 @@
|
||||
package org.libremediaconverter.join
|
||||
|
||||
import android.app.Application
|
||||
import android.net.Uri
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.work.workDataOf
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.convert.ConversionDependencies
|
||||
import org.libremediaconverter.convert.ParkedPickDispatcher
|
||||
import org.libremediaconverter.convert.RecordingPublisher
|
||||
import org.libremediaconverter.convert.installTestWorkManager
|
||||
import org.libremediaconverter.work.ConcatWorker
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
|
||||
/**
|
||||
* `PickOwnershipTest`'s case on the join side.
|
||||
*
|
||||
* `onInputsPicked` makes one write and it lands after a hop off the main thread, so it belongs to
|
||||
* whichever pick was in flight rather than to whichever set of files the user last chose. Two
|
||||
* selections in quick succession — likelier here than on the convert side, since a join picks
|
||||
* several files at a time and the metadata query is per file — put the loser's files on screen if
|
||||
* its query came back second.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class JoinPickOwnershipTest {
|
||||
|
||||
private lateinit var app: Application
|
||||
private lateinit var parkedPick: ParkedPickDispatcher
|
||||
private lateinit var viewModel: JoinViewModel
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
app = RuntimeEnvironment.getApplication()
|
||||
ConversionDependencies.publisher = { RecordingPublisher(app) }
|
||||
installTestWorkManager(app, workDataOf(ConcatWorker.KEY_OUTPUT_PATH to "/dev/null"))
|
||||
|
||||
parkedPick = ParkedPickDispatcher()
|
||||
viewModel = JoinViewModel(app, pickDispatcher = parkedPick)
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() {
|
||||
ConversionDependencies.reset()
|
||||
}
|
||||
|
||||
/**
|
||||
* Two selections, with the first one's metadata query the slow one.
|
||||
*
|
||||
* The order is chosen rather than raced: both queries are parked, and this runs the second
|
||||
* before the first.
|
||||
*/
|
||||
@Test
|
||||
fun `the slower of two selections does not land on top of the faster one`() {
|
||||
viewModel.onInputsPicked(FIRST)
|
||||
viewModel.onInputsPicked(SECOND)
|
||||
|
||||
val queries = parkedPick.takeParked()
|
||||
assertEquals("both selections should be in flight", 2, queries.size)
|
||||
// The second selection's query comes back first; the first one's is the straggler.
|
||||
queries[1].run()
|
||||
queries[0].run()
|
||||
|
||||
val current = viewModel.state.value
|
||||
assertEquals(
|
||||
"a selection the user has already replaced took the screen: $current",
|
||||
SECOND,
|
||||
(current as JoinState.Ready).inputs.map { it.uri },
|
||||
)
|
||||
}
|
||||
|
||||
private companion object {
|
||||
val FIRST = listOf(
|
||||
Uri.parse("content://test/first-a.mp4"),
|
||||
Uri.parse("content://test/first-b.mp4"),
|
||||
)
|
||||
val SECOND = listOf(
|
||||
Uri.parse("content://test/second-a.mp4"),
|
||||
Uri.parse("content://test/second-b.mp4"),
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,166 @@
|
||||
package org.libremediaconverter.join
|
||||
|
||||
import android.app.Application
|
||||
import android.net.Uri
|
||||
import androidx.media3.common.util.UnstableApi
|
||||
import androidx.work.WorkManager
|
||||
import androidx.work.workDataOf
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.convert.ConversionDependencies
|
||||
import org.libremediaconverter.convert.ParkedPickDispatcher
|
||||
import org.libremediaconverter.convert.RecordingPublisher
|
||||
import org.libremediaconverter.convert.awaitState
|
||||
import org.libremediaconverter.convert.installTestWorkManager
|
||||
import org.libremediaconverter.model.ConcatStrategy
|
||||
import org.libremediaconverter.work.ConcatWorker
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import java.io.File
|
||||
|
||||
/**
|
||||
* Issue #49 on the join side, where nothing was watching for it.
|
||||
*
|
||||
* `reattach()` checks that the screen is still free, and then hands the answer to `observe()`,
|
||||
* which writes from a *different* coroutine that has to suspend on `collect` before it can write
|
||||
* anything at all. So the check happens at one moment and the write lands at another, with a
|
||||
* whole pick able to fit in between:
|
||||
*
|
||||
* 1. `init` starts the tag query and suspends in it.
|
||||
* 2. The user picks files; `onInputsPicked` suspends in its metadata query.
|
||||
* 3. The query comes back. The screen is still `Idle` — step 2 has not written yet — so the
|
||||
* guard passes and an observation of the old job is launched.
|
||||
* 4. The pick lands. `Ready(picked)`. The user owns the screen.
|
||||
* 5. The observation's first `WorkInfo` arrives and writes `Joined(yesterday's file)` over it.
|
||||
*
|
||||
* The comment above that guard used to say "no suspension point between this check and the
|
||||
* assignment below, so nothing can interleave". There is no assignment below, and the two lines
|
||||
* are in different coroutines.
|
||||
*
|
||||
* The convert side has been failing this on CI for two days — four occurrences across three API
|
||||
* levels, each read as flaky infrastructure. `JoinViewModel` has the identical shape and no test
|
||||
* at all, which is why this one was written before the fix rather than after it.
|
||||
*/
|
||||
@UnstableApi
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class JoinReattachmentOwnershipTest {
|
||||
|
||||
private lateinit var app: Application
|
||||
private lateinit var publisher: RecordingPublisher
|
||||
private lateinit var workManager: WorkManager
|
||||
private lateinit var staged: File
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
app = RuntimeEnvironment.getApplication()
|
||||
publisher = RecordingPublisher(app)
|
||||
ConversionDependencies.publisher = { publisher }
|
||||
|
||||
staged = publisher.createStagingFile("joined-yesterday.mp4").apply { writeBytes(ByteArray(4096)) }
|
||||
installTestWorkManager(
|
||||
app,
|
||||
workDataOf(
|
||||
ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath,
|
||||
ConcatWorker.KEY_STRATEGY to ConcatStrategy.STREAM_COPY.name,
|
||||
),
|
||||
)
|
||||
workManager = WorkManager.getInstance(app)
|
||||
// The situation reattachment exists for: a join that finished in a process that is gone,
|
||||
// with its output still in the cache and nothing in the UI holding its id.
|
||||
workManager.enqueue(
|
||||
ConcatWorker.request(
|
||||
inputs = listOf(
|
||||
Uri.parse("content://test/yesterday-a.mp4"),
|
||||
Uri.parse("content://test/yesterday-b.mp4"),
|
||||
),
|
||||
totalBytes = 8_192L,
|
||||
),
|
||||
).result.get()
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() {
|
||||
ConversionDependencies.reset()
|
||||
}
|
||||
|
||||
/**
|
||||
* The race, made into a state the test can sit in rather than one it has to catch.
|
||||
*
|
||||
* The pick is parked on a dispatcher this test owns, so it is in flight — issued, not yet
|
||||
* written — for as long as the assertions need it to be. Everything else is real: a real
|
||||
* `WorkManager` holding a real finished job, the production `reattach`, the production
|
||||
* `observe`.
|
||||
*
|
||||
* Determinism comes from where Robolectric leaves the main looper. `reattach`'s tag query
|
||||
* hops to a real [kotlinx.coroutines.Dispatchers.IO] thread, so its continuation can only
|
||||
* come back as a message posted to the main looper — and that looper is paused, so it cannot
|
||||
* run until something pumps it. `onInputsPicked` is an ordinary synchronous call from this
|
||||
* thread. The pick is therefore always issued before the guard runs, with nothing left to
|
||||
* timing.
|
||||
*/
|
||||
@Test
|
||||
fun `a join found while the user was picking never reaches the screen`() {
|
||||
val parkedPick = ParkedPickDispatcher()
|
||||
val viewModel = JoinViewModel(app, pickDispatcher = parkedPick)
|
||||
viewModel.onInputsPicked(PICKED)
|
||||
|
||||
assertEquals(
|
||||
"the pick must still be in flight, or this proves something about a different situation",
|
||||
1,
|
||||
parkedPick.parkedCount,
|
||||
)
|
||||
|
||||
// The control, and the reason this test does not rest on a settle window being long
|
||||
// enough. A second ViewModel with nothing to supersede it reattaches to the same job
|
||||
// through the same code; when it has arrived, the whole query-guard-observe-write path
|
||||
// has demonstrably run to completion. `viewModel` started its own reattachment first, so
|
||||
// it has had at least as long. Waiting on this rather than on a sleep is what makes the
|
||||
// assertion below "it did not happen" instead of "it had not happened yet".
|
||||
reattachmentHasRunToCompletion()
|
||||
|
||||
val current = viewModel.state.value
|
||||
assertTrue("reattachment took the screen from the user: $current", current is JoinState.Idle)
|
||||
|
||||
// And the pick, when it lands, is what stays there.
|
||||
parkedPick.runAll()
|
||||
val ready = awaitState(viewModel.state, "Ready") { it is JoinState.Ready }
|
||||
assertEquals(PICKED, (ready as JoinState.Ready).inputs.map { it.uri })
|
||||
reattachmentHasRunToCompletion()
|
||||
assertEquals("the user's pick must survive a late reattachment", ready, viewModel.state.value)
|
||||
}
|
||||
|
||||
/**
|
||||
* The other half of the contract: a reattachment nobody has superseded still takes the screen.
|
||||
*
|
||||
* Without this, dropping every reattachment on the floor would pass the test above. It is the
|
||||
* same job, the same WorkManager and the same production path — only the pick is missing.
|
||||
*/
|
||||
@Test
|
||||
fun `a join nobody has superseded still reaches the screen`() {
|
||||
val joined = awaitState(JoinViewModel(app).state, "Joined") { it is JoinState.Joined }
|
||||
|
||||
assertEquals(staged.absolutePath, (joined as JoinState.Joined).staged.absolutePath)
|
||||
}
|
||||
|
||||
/**
|
||||
* Drives a throwaway ViewModel through a whole reattachment, and returns once it has landed.
|
||||
*
|
||||
* [awaitState] pumps the main looper, which is what runs every reattachment continuation
|
||||
* waiting on it — this one's and the one belonging to the ViewModel under test, which was
|
||||
* posted earlier and therefore runs first.
|
||||
*/
|
||||
private fun reattachmentHasRunToCompletion() {
|
||||
awaitState(JoinViewModel(app).state, "Joined") { it is JoinState.Joined }
|
||||
}
|
||||
|
||||
private companion object {
|
||||
val PICKED = listOf(
|
||||
Uri.parse("content://test/clip-one.mp4"),
|
||||
Uri.parse("content://test/clip-two.mp4"),
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -18,6 +18,7 @@ import org.junit.Rule
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.libremediaconverter.convert.InputFile
|
||||
import org.libremediaconverter.convert.PendingSave
|
||||
import org.libremediaconverter.model.ConcatStrategy
|
||||
import org.libremediaconverter.ui.TestTags
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
@@ -219,6 +220,51 @@ class JoinStateAffordancesTest {
|
||||
assertEquals(listOf("reset"), events)
|
||||
}
|
||||
|
||||
/**
|
||||
* The negative that bounds #30 on this screen. A join that died staged nothing, so its `Failed`
|
||||
* carries no [PendingSave] and there is nothing a save dialog could be handed. A retry button
|
||||
* rendered unconditionally here could only fail, and this is what notices.
|
||||
*/
|
||||
@Test
|
||||
fun `a failed join offers no way to save`() {
|
||||
setContent(JoinState.Failed(message = "The second file has no audio track, so joining stopped."))
|
||||
|
||||
composeRule.onNodeWithTag(TestTags.RETRY_SAVE).assertDoesNotExist()
|
||||
composeRule.onNodeWithTag(TestTags.SAVE_FILE).assertDoesNotExist()
|
||||
}
|
||||
|
||||
/**
|
||||
* #30 on this screen: `save()` keeps the staged file when the copy out throws, and until this
|
||||
* branch grew a second button the only control it rendered was "Start over" -- `reset()`, which
|
||||
* deletes exactly that file. Both, not one: the restart still has to be reachable.
|
||||
*/
|
||||
@Test
|
||||
fun `a failed join save offers the file again as well as a restart`() {
|
||||
setContent(failedSave())
|
||||
|
||||
composeRule.onNodeWithTag(TestTags.RETRY_SAVE).assertExists()
|
||||
composeRule.onNodeWithTag(TestTags.START_OVER).assertExists()
|
||||
}
|
||||
|
||||
/** The name comes from the job, so a retry wired to a literal would hand back the wrong one. */
|
||||
@Test
|
||||
fun `tapping try saving again hands back the name the join chose`() {
|
||||
setContent(failedSave())
|
||||
|
||||
composeRule.onNodeWithTag(TestTags.RETRY_SAVE).performScrollTo().performClick()
|
||||
|
||||
assertEquals(listOf("save:joined.mp4"), events)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `tapping start over after a failed join save resets and does not save`() {
|
||||
setContent(failedSave())
|
||||
|
||||
composeRule.onNodeWithTag(TestTags.START_OVER).performScrollTo().performClick()
|
||||
|
||||
assertEquals(listOf("reset"), events)
|
||||
}
|
||||
|
||||
/** Anything `FileRow` tagged, whichever file it is showing. The prefix comes from the table. */
|
||||
private val isFileRow = SemanticsMatcher("is a join file row") { node ->
|
||||
node.config.getOrNull(SemanticsProperties.TestTag)?.startsWith(TestTags.Join.fileRow("")) == true
|
||||
@@ -230,6 +276,19 @@ class JoinStateAffordancesTest {
|
||||
sizeBytes = 4_000_000L,
|
||||
)
|
||||
|
||||
/**
|
||||
* A `Failed` an earlier save left carrying its file, which is the only way `retry` is non-null.
|
||||
* `staged` names a missing file for the same reason [joined] does -- this arm reads no length.
|
||||
*/
|
||||
private fun failedSave() = JoinState.Failed(
|
||||
message = "There was not enough room on the destination.",
|
||||
retry = PendingSave(
|
||||
staged = File("no-such-staged-output.mp4"),
|
||||
suggestedName = "joined.mp4",
|
||||
mimeType = "video/mp4",
|
||||
),
|
||||
)
|
||||
|
||||
/** `staged` names a missing file deliberately -- see the same helper in `JoinScreenContentTest`. */
|
||||
private fun joined(strategy: ConcatStrategy) = JoinState.Joined(
|
||||
staged = File("no-such-staged-output.mp4"),
|
||||
|
||||
@@ -11,6 +11,11 @@ import org.junit.Test
|
||||
* `OutputFormat` used to be twelve hand-picked triples, and its KDoc defended that on the grounds
|
||||
* that a closed set was what made routing decidable. Opening it up moves that burden here, so this
|
||||
* is where decidability now has to be proven.
|
||||
*
|
||||
* That includes what a refusal offers instead. `Validation.Invalid` promises every suggestion is
|
||||
* itself valid and names this class as the proof, so a branch that assembles its own suggestion
|
||||
* list rather than going through `suggestions()` is only checked here if some row happens to reach
|
||||
* it — which is how a dead-end chip survived two widenings of that table.
|
||||
*/
|
||||
class ContainerCapabilitiesTest {
|
||||
|
||||
@@ -20,6 +25,44 @@ class ContainerCapabilitiesTest {
|
||||
container = Container.MP4,
|
||||
)
|
||||
|
||||
/**
|
||||
* An MP3, and the reason several rules below need a second probe.
|
||||
*
|
||||
* `hasVideo = false` is the load-bearing field. Every rule that reads only the spec answers the
|
||||
* same for this input as for a video file, which is exactly how a spec naming a video codec was
|
||||
* called valid for a file with no video track to put in it.
|
||||
*/
|
||||
private val mp3Source = InputProbe(
|
||||
videoCodec = null,
|
||||
audioCodec = "mp3",
|
||||
hasVideo = false,
|
||||
kind = InputKind.AUDIO_ONLY,
|
||||
container = Container.MP3,
|
||||
)
|
||||
|
||||
/**
|
||||
* An audio-only input carrying a codec MP4 has no place for at all.
|
||||
*
|
||||
* Vorbis lives in Ogg and Matroska; MP4 carries AAC, MP3, Opus and FLAC. That gap is what turns
|
||||
* a suggestion which merely drops the video track into a second refusal.
|
||||
*/
|
||||
private val vorbisSource = InputProbe(
|
||||
videoCodec = null,
|
||||
audioCodec = "vorbis",
|
||||
hasVideo = false,
|
||||
kind = InputKind.AUDIO_ONLY,
|
||||
container = Container.OGG,
|
||||
)
|
||||
|
||||
/** The same shape, for the other codec MP4 refuses. One case is a coincidence; two is the rule. */
|
||||
private val pcmSource = InputProbe(
|
||||
videoCodec = null,
|
||||
audioCodec = "pcm_s16le",
|
||||
hasVideo = false,
|
||||
kind = InputKind.AUDIO_ONLY,
|
||||
container = Container.WAV,
|
||||
)
|
||||
|
||||
// --- copy and encode are different questions ----------------------------
|
||||
|
||||
/**
|
||||
@@ -82,20 +125,56 @@ class ContainerCapabilitiesTest {
|
||||
}
|
||||
}
|
||||
|
||||
/** A suggestion that is itself invalid is worse than no suggestion. */
|
||||
/**
|
||||
* A suggestion that is itself invalid is worse than no suggestion.
|
||||
*
|
||||
* Only a branch that assembles its own suggestion list can break that promise: [suggestions]
|
||||
* ends by filtering on `validate(...).isValid`, so everything routed through it is valid by
|
||||
* construction. Those branches are what this table has to cover — the image output, and copy
|
||||
* the video from a file that has none, which built its list by hand and came back refused for
|
||||
* a Vorbis or PCM source into MP4 and an MP3 into WebM. The Advanced picker showed a one-tap
|
||||
* fix that led straight to a second error, through two widenings of this table that never
|
||||
* reached the branch.
|
||||
*/
|
||||
@Test
|
||||
fun `every suggestion is itself valid`() {
|
||||
val broken = OutputSpec(Container.WEBM, VideoCodec.H264, AudioCodec.AAC)
|
||||
val result = ContainerCapabilities.validate(broken, h264Source)
|
||||
val cases = listOf(
|
||||
OutputSpec(Container.WEBM, VideoCodec.H264, AudioCodec.AAC) to h264Source,
|
||||
// The audio-only input. Every rejection it can reach used to hand back `None + None`
|
||||
// — a spec validation refuses in the next breath — because these branches built their
|
||||
// suggestion by hand instead of going through the repair-and-filter path.
|
||||
OutputSpec(Container.MP4, VideoCodec.H265, AudioCodec.NONE) to mp3Source,
|
||||
OutputSpec(Container.MP4, VideoCodec.COPY, AudioCodec.NONE) to mp3Source,
|
||||
OutputSpec(Container.MP4, VideoCodec.NONE, AudioCodec.NONE) to mp3Source,
|
||||
OutputSpec(Container.MP4, VideoCodec.COPY, AudioCodec.AAC) to mp3Source,
|
||||
// Copy-the-video-from-a-file-with-no-video, the last branch that built its offer by
|
||||
// hand. It escaped the five rows above because `spec.copy(videoCodec = NONE)` is valid
|
||||
// exactly when the audio axis happens to be fine — true for the AAC and MP3 sources
|
||||
// used there, false for any audio the target container cannot carry.
|
||||
OutputSpec(Container.MP4, VideoCodec.COPY, AudioCodec.COPY) to vorbisSource,
|
||||
OutputSpec(Container.MP4, VideoCodec.COPY, AudioCodec.COPY) to pcmSource,
|
||||
OutputSpec(Container.WEBM, VideoCodec.COPY, AudioCodec.COPY) to mp3Source,
|
||||
// The same branch with audio the container *can* hold, which is the half that already
|
||||
// worked and must keep working: the repair here is a copy, so the offer is the very
|
||||
// spec the caller handed to `suggestions`. It survives only because the exclusion is
|
||||
// against what the user asked for rather than against the repair.
|
||||
OutputSpec(Container.MP4, VideoCodec.COPY, AudioCodec.COPY) to mp3Source,
|
||||
// The one branch that still builds its list by hand, so that it is asserted rather
|
||||
// than merely reasoned about: an image container takes `None + None` and nothing else,
|
||||
// which makes its single offer valid by construction.
|
||||
OutputSpec(Container.GIF, VideoCodec.H264, AudioCodec.AAC) to h264Source,
|
||||
)
|
||||
|
||||
val invalid = result as? Validation.Invalid
|
||||
?: throw AssertionError("expected H.264 in WebM to be rejected")
|
||||
assertTrue("no alternatives offered", invalid.suggestions.isNotEmpty())
|
||||
invalid.suggestions.forEach { suggestion ->
|
||||
assertTrue(
|
||||
"suggested $suggestion is itself invalid",
|
||||
ContainerCapabilities.validate(suggestion, h264Source).isValid,
|
||||
)
|
||||
cases.forEach { (spec, probe) ->
|
||||
val invalid = ContainerCapabilities.validate(spec, probe) as? Validation.Invalid
|
||||
?: throw AssertionError("expected $spec to be rejected")
|
||||
assertTrue("no alternatives offered for $spec on $probe", invalid.suggestions.isNotEmpty())
|
||||
invalid.suggestions.forEach { suggestion ->
|
||||
assertTrue(
|
||||
"suggested $suggestion for $spec on $probe is itself invalid",
|
||||
ContainerCapabilities.validate(suggestion, probe).isValid,
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -127,6 +206,117 @@ class ContainerCapabilitiesTest {
|
||||
assertTrue((result as Validation.Invalid).suggestions.isNotEmpty())
|
||||
}
|
||||
|
||||
/**
|
||||
* The same rule, seen only against the probe.
|
||||
*
|
||||
* A video codec named for a file with no video track is dropped, not encoded — so
|
||||
* MP4/H.265/None on an MP3 empties the output exactly as None/None does. Reading the spec
|
||||
* alone answered "valid" because the spec names a video codec, and the job went to Media3,
|
||||
* where `EditedMediaItem.Builder` refuses a composition with both tracks removed by throwing
|
||||
* on Transformer's own HandlerThread.
|
||||
*/
|
||||
@Test
|
||||
fun `a video codec named for a file with no video track and no audio is refused`() {
|
||||
ContainerCapabilities.encodableVideo(Container.MP4).forEach { codec ->
|
||||
val spec = OutputSpec(Container.MP4, codec, AudioCodec.NONE)
|
||||
val result = ContainerCapabilities.validate(spec, mp3Source)
|
||||
|
||||
assertFalse(
|
||||
"MP4/${codec.label}/None on an audio-only input plans to (Drop, Drop) and would " +
|
||||
"produce an empty file; it must be refused. Got $result",
|
||||
result.isValid,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The refusal is only worth having if it leads somewhere.
|
||||
*
|
||||
* The COPY form of this was already refused, but its one hand-built suggestion was
|
||||
* `None + None` — which validation refuses in the next breath, so the Advanced picker offered
|
||||
* a one-tap fix that fixed nothing. Every face of the rule now goes through the shared
|
||||
* suggestion path, so the offer keeps the one track the input actually has.
|
||||
*/
|
||||
@Test
|
||||
fun `refusing an empty output still offers a way to keep the audio`() {
|
||||
listOf(VideoCodec.H265, VideoCodec.H264, VideoCodec.COPY, VideoCodec.NONE).forEach { codec ->
|
||||
val spec = OutputSpec(Container.MP4, codec, AudioCodec.NONE)
|
||||
val invalid = ContainerCapabilities.validate(spec, mp3Source) as? Validation.Invalid
|
||||
?: throw AssertionError("expected MP4/${codec.label}/None to be rejected")
|
||||
|
||||
assertTrue(
|
||||
"a refusal with no way out is a dead end in the Advanced picker",
|
||||
invalid.suggestions.isNotEmpty(),
|
||||
)
|
||||
assertTrue(
|
||||
"every suggestion must keep a track, got ${invalid.suggestions}",
|
||||
invalid.suggestions.all { it.audioCodec != AudioCodec.NONE },
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A repair must not name a track the input does not have.
|
||||
*
|
||||
* `repairVideo` used to fall through to "the first codec this container can encode" whenever
|
||||
* nothing else fitted, and for an MP3 that produced the non-sequitur `MP4 · H.264 · Copy`.
|
||||
* It validated, so nothing caught it — but [CopyPlanner] drops that video track anyway, which
|
||||
* makes the codec in the offer a fiction.
|
||||
*/
|
||||
@Test
|
||||
fun `a repair for a file with no video track never names a video codec`() {
|
||||
listOf(
|
||||
OutputSpec(Container.MP4, VideoCodec.H265, AudioCodec.NONE),
|
||||
OutputSpec(Container.MP4, VideoCodec.NONE, AudioCodec.NONE),
|
||||
OutputSpec(Container.MP4, VideoCodec.COPY, AudioCodec.NONE),
|
||||
).forEach { spec ->
|
||||
val invalid = ContainerCapabilities.validate(spec, mp3Source) as Validation.Invalid
|
||||
invalid.suggestions.forEach {
|
||||
assertEquals(
|
||||
"offering ${it.videoCodec.label} for a file with no video track is a fiction; " +
|
||||
"CopyPlanner drops it. Suggested $it for $spec",
|
||||
VideoCodec.NONE,
|
||||
it.videoCodec,
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The rule stated as the property it is, over the whole matrix.
|
||||
*
|
||||
* A plan of (Drop, Drop) is precisely the composition `EditedMediaItem.Builder` refuses to
|
||||
* build, so no non-image spec that reaches it may be called valid. Sweeping every container ×
|
||||
* codec × codec against both probes is what stops the next container or codec from
|
||||
* reintroducing the gap on an axis nobody thought to write a case for.
|
||||
*
|
||||
* Image outputs are exempt and deliberately so: GIF and PNG frames carry no codecs at all, and
|
||||
* `None + None` is the only spec they accept — but they never reach Media3, because the router
|
||||
* sends every image output to FFmpeg.
|
||||
*/
|
||||
@Test
|
||||
fun `no valid non-image spec plans to remove both tracks`() {
|
||||
val specs = Container.entries
|
||||
.filterNot { it == Container.GIF || it == Container.IMAGE_SEQUENCE }
|
||||
.flatMap { container -> VideoCodec.entries.map { container to it } }
|
||||
.flatMap { (container, video) -> AudioCodec.entries.map { OutputSpec(container, video, it) } }
|
||||
val cases = specs.flatMap { spec -> listOf(h264Source, mp3Source).map { spec to it } }
|
||||
|
||||
val empties = cases.filter { (spec, probe) ->
|
||||
val plan = CopyPlanner.plan(spec, probe)
|
||||
plan.video == VideoPlan.Drop && plan.audio == AudioPlan.Drop
|
||||
}
|
||||
|
||||
assertTrue("the sweep found nothing to check — the filter has gone wrong", empties.isNotEmpty())
|
||||
empties.forEach { (spec, probe) ->
|
||||
assertFalse(
|
||||
"$spec on $probe plans to (Drop, Drop) — an empty file, and the composition " +
|
||||
"Media3 cannot build — so it must not validate",
|
||||
ContainerCapabilities.validate(spec, probe).isValid,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `copying is offered as the fix when the codec is right but unencodable`() {
|
||||
val av1Source = InputProbe(videoCodec = "av1", audioCodec = "aac", container = Container.MKV)
|
||||
|
||||
@@ -350,6 +350,34 @@ class ConversionRouterTest {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Why `Media3Engine` still needs a guard of its own.
|
||||
*
|
||||
* `ContainerCapabilities.validate` now refuses "a video codec with the audio off" for an input
|
||||
* with no video track, so neither the picker nor `ConversionWorker` will start one. Routing is
|
||||
* a separate question and still answers MEDIA3 — nothing about a dropped track makes the job
|
||||
* un-hardware-able — so a request that skips validation, from a direct
|
||||
* `ConversionWorker.request(...)` or a job queued before the settings changed, arrives at the
|
||||
* engine with a plan Media3 cannot build. That has to fail the job, not the process.
|
||||
*/
|
||||
@Test
|
||||
fun `a plan that drops both tracks still routes to media3`() {
|
||||
val audioOnly = InputProbe(
|
||||
videoCodec = null,
|
||||
audioCodec = "mp3",
|
||||
hasVideo = false,
|
||||
container = Container.MP3,
|
||||
kind = InputKind.AUDIO_ONLY,
|
||||
)
|
||||
val spec = OutputSpec(Container.MP4, VideoCodec.H265, AudioCodec.NONE)
|
||||
|
||||
val plan = CopyPlanner.plan(spec, audioOnly)
|
||||
assertEquals(VideoPlan.Drop, plan.video)
|
||||
assertEquals(AudioPlan.Drop, plan.audio)
|
||||
|
||||
assertEquals(Engine.MEDIA3, route(spec, probe = audioOnly).engine)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `audio-only formats are flagged as such`() {
|
||||
assertEquals(true, OutputFormat.MP3.isAudioOnly)
|
||||
|
||||
@@ -146,6 +146,33 @@ class CopyPlannerTest {
|
||||
assertTrue("copying the only track is still a remux", plan.isPureRemux)
|
||||
}
|
||||
|
||||
/**
|
||||
* The one plan `Media3Engine` cannot be handed.
|
||||
*
|
||||
* `EditedMediaItem.Builder` refuses a composition with both tracks removed —
|
||||
* checkState("Audio and video cannot both be removed") — and this is how an ordinary-looking
|
||||
* spec reaches it: a video codec named for a file that has no video, with the audio switched
|
||||
* off. Neither half is unusual on its own, which is why validation could read the spec, see a
|
||||
* video codec, and call it fine.
|
||||
*/
|
||||
@Test
|
||||
fun `an audio-only source with the audio dropped removes both tracks`() {
|
||||
val audioOnly = InputProbe(
|
||||
videoCodec = null,
|
||||
audioCodec = "mp3",
|
||||
hasVideo = false,
|
||||
container = Container.MP3,
|
||||
kind = InputKind.AUDIO_ONLY,
|
||||
)
|
||||
val plan = CopyPlanner.plan(
|
||||
OutputSpec(Container.MP4, VideoCodec.H265, AudioCodec.NONE),
|
||||
audioOnly,
|
||||
)
|
||||
assertEquals(VideoPlan.Drop, plan.video)
|
||||
assertEquals(AudioPlan.Drop, plan.audio)
|
||||
assertTrue("an empty plan is not a remux", !plan.isPureRemux)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `copying one track and encoding the other is not a pure remux`() {
|
||||
val plan = CopyPlanner.plan(
|
||||
|
||||
@@ -0,0 +1,131 @@
|
||||
package org.libremediaconverter.ui.theme
|
||||
|
||||
import androidx.compose.material3.ColorScheme
|
||||
import androidx.compose.material3.MaterialTheme
|
||||
import androidx.compose.ui.graphics.luminance
|
||||
import androidx.compose.ui.test.junit4.v2.createComposeRule
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertNotEquals
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Rule
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
|
||||
/**
|
||||
* The theme has to resolve the scheme its arguments name, and `ThemeKt` had no test at all --
|
||||
* 23 lines, none of them covered, which is how #68 was found.
|
||||
*
|
||||
* Only two of the four branches in [LibreMediaConverterTheme]'s `when` are reachable from the
|
||||
* app. `MainActivity` is the single call site and passes no arguments, so `dynamicColor` is
|
||||
* always `true` and the live choice is between the dynamic dark and dynamic light schemes.
|
||||
* Those two are what ships, and asserting on them survives whichever way #68 is decided.
|
||||
*
|
||||
* **The other two branches have no caller.** `dynamicColor = false` is passed below by this
|
||||
* test and by nothing else in `app/src`, so the coverage it produces is not evidence that a
|
||||
* switch exists -- misreading it that way is the whole reason #68 was filed. #68 is the open
|
||||
* decision about whether one ever will exist.
|
||||
*
|
||||
* What the assertions distinguish the branches on was measured under Robolectric `sdk=36`
|
||||
* rather than assumed. The dynamic palette resolves to the platform's own default there --
|
||||
* dark background `#121318` against light `#FAF8FF`, dark primary `#B0C6FF` -- and that is a
|
||||
* different hue from the brand palette's [Purple80] / [Purple40]. A dynamic scheme reads the
|
||||
* device, so those exact values belong to the Robolectric stub and to no particular phone,
|
||||
* which is why the live-branch tests compare the two resolved schemes against each other
|
||||
* instead of hard-coding either one.
|
||||
*/
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class ThemeColorSchemeTest {
|
||||
|
||||
// The **v2** rule (`androidx.compose.ui.test.junit4.v2`), as everywhere else in this
|
||||
// source set.
|
||||
@get:Rule
|
||||
val composeRule = createComposeRule()
|
||||
|
||||
/**
|
||||
* Every scheme the `when` can produce, read out of [MaterialTheme] inside the content
|
||||
* lambda -- the only place that shows what the theme actually chose, rather than what the
|
||||
* caller hoped for.
|
||||
*
|
||||
* All four are resolved in one composition because `setContent` may be called once per
|
||||
* test, and a comparison needs at least two of them.
|
||||
*/
|
||||
private fun resolveAll(): Schemes {
|
||||
lateinit var dynamicDark: ColorScheme
|
||||
lateinit var dynamicLight: ColorScheme
|
||||
lateinit var brandDark: ColorScheme
|
||||
lateinit var brandLight: ColorScheme
|
||||
composeRule.setContent {
|
||||
LibreMediaConverterTheme(darkTheme = true) { dynamicDark = MaterialTheme.colorScheme }
|
||||
LibreMediaConverterTheme(darkTheme = false) { dynamicLight = MaterialTheme.colorScheme }
|
||||
LibreMediaConverterTheme(darkTheme = true, dynamicColor = false) {
|
||||
brandDark = MaterialTheme.colorScheme
|
||||
}
|
||||
LibreMediaConverterTheme(darkTheme = false, dynamicColor = false) {
|
||||
brandLight = MaterialTheme.colorScheme
|
||||
}
|
||||
}
|
||||
composeRule.waitForIdle()
|
||||
return Schemes(dynamicDark, dynamicLight, brandDark, brandLight)
|
||||
}
|
||||
|
||||
private class Schemes(
|
||||
val dynamicDark: ColorScheme,
|
||||
val dynamicLight: ColorScheme,
|
||||
val brandDark: ColorScheme,
|
||||
val brandLight: ColorScheme,
|
||||
)
|
||||
|
||||
/**
|
||||
* The live branches, and the one assertion that catches them being swapped: both dynamic
|
||||
* schemes come from the same device palette, so they are similar enough that identity or a
|
||||
* bare inequality would prove nothing. Background luminance is not similar -- it is the
|
||||
* thing dark mode is for.
|
||||
*/
|
||||
@Test
|
||||
fun `dark mode resolves a darker scheme than light mode`() {
|
||||
val schemes = resolveAll()
|
||||
|
||||
val dark = schemes.dynamicDark.background.luminance()
|
||||
val light = schemes.dynamicLight.background.luminance()
|
||||
assertTrue(
|
||||
"darkTheme = true should resolve the dynamic dark scheme, whose background " +
|
||||
"luminance ($dark) is below the light scheme's ($light)",
|
||||
dark < light,
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Which of the two dark branches ran, and which of the two light ones: the scheme a live
|
||||
* call resolves is the dynamic one, not the brand palette sitting next to it.
|
||||
*/
|
||||
@Test
|
||||
fun `the live branches take the dynamic palette rather than the brand one`() {
|
||||
val schemes = resolveAll()
|
||||
|
||||
assertNotEquals(
|
||||
"the default dynamicColor = true should not resolve the brand dark palette",
|
||||
Purple80,
|
||||
schemes.dynamicDark.primary,
|
||||
)
|
||||
assertNotEquals(
|
||||
"the default dynamicColor = true should not resolve the brand light palette",
|
||||
Purple40,
|
||||
schemes.dynamicLight.primary,
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* The two dead branches. Nothing in `app/src` passes `dynamicColor = false`; this test
|
||||
* does it directly, because the parameter is public, and that is the only way either
|
||||
* branch runs. Covered so a later decision on #68 starts from a tested `when` -- not
|
||||
* because the brand palette is reachable in the app.
|
||||
*/
|
||||
@Test
|
||||
fun `the brand palette branches run only when dynamicColor is passed explicitly`() {
|
||||
val schemes = resolveAll()
|
||||
|
||||
assertEquals(Purple80, schemes.brandDark.primary)
|
||||
assertEquals(Purple40, schemes.brandLight.primary)
|
||||
}
|
||||
}
|
||||
@@ -32,8 +32,12 @@ reached* is not. Re-measured on 2026-08-22, seven runs, one variable at a time:
|
||||
| r06 | `android-37.1` rev 8 | `swangle_indirect` | ANGLE | **yes, 285 s** | 23 |
|
||||
| r07 | `android-37.0` rev 6 | `host` + `-feature -HostComposition` | host | **no**, wedged adb at 208 s | not readable |
|
||||
|
||||
The discriminator is exact across all seven: **a run boots if and only if the emulator log says
|
||||
something other than `gles_mode_selected:host`.**
|
||||
The discriminator is exact across the **six runs that reported**: a run boots if and only if the
|
||||
emulator log says something other than `gles_mode_selected:host`. r07 is excluded on purpose — it
|
||||
wedged adb at 208 s and is recorded below as inconclusive rather than ruled out, and a row this
|
||||
page calls inconclusive cannot also be counted as evidence. Excluding it costs nothing: r07 is a
|
||||
`host` row, so the discriminator predicts it would not boot, and confirming a prediction with the
|
||||
one run whose evidence did not come back would add no information either way.
|
||||
|
||||
One caveat about how independent those rows are, because the table flatters itself. `-gpu
|
||||
angle_indirect` (r05) and `-gpu swangle_indirect` (r03) both logged `gles_mode_selected:swangle`
|
||||
@@ -509,8 +513,9 @@ API 37", not "is that codec broken".
|
||||
|
||||
`.github/workflows/api37-debug.yml` carried "roughly every 20 s" for the kill cycle in its own
|
||||
comments. That number was the watchdog's **sampling** interval, not the cadence, and the two got
|
||||
conflated. Measured across the seven runs above, gaps between successive `hasReadColorBufferDma`
|
||||
aborts run **20 s to 90 s, median 60–70 s — three to five aborts in a four-minute window**.
|
||||
conflated. Measured across the **six runs whose crash buffer could be read** — r07 wedged adb
|
||||
before one could be taken, so it contributes no gaps — successive `hasReadColorBufferDma` aborts
|
||||
run **20 s to 90 s, median 60–70 s — three to five aborts in a four-minute window**.
|
||||
Slower than assumed, and still not slow enough: install, data-directory creation and
|
||||
instrumentation start-up do not fit inside one gap.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user