Skip to content

Commit e38ec28

Browse files
cmitroutautconyOrpheeGT
authored
07222026 (#1)
* feat: replace bash scripts with Python equivalents for copyright checks and test coverage * feat: refactor Rheingold DatabaseProvider and LicenseManagement Models * feat: improve reportgenerator handling and error reporting in test coverage script * test: enhance test coverage and refactor utility methods * feat: add --enable-ista-voltage option to inject an ISTAVoltageControl.dll (tautcony#88) Hooks into IstaOperation.exe's InitializeServices, loading ISTAVoltageControl.dll at runtime and calling Controller.StartFromIstaOperation to enable KL15/KL30 voltage tracking over OBD/D-CAN or ENET. The injection is idempotent and wrapped in a silent try/catch so it never crashes ISTA. IMPORTANT — two external dependencies are required, neither of which is part of this repository: 1. EdiabasLib (Api64.dll) The DLL is resolved at runtime via the BinPathModifications registry key (HKLM\SOFTWARE\BMWGroup\ISPI\Rheingold\BMW.Rheingold.ISTAGUI .BinPathModifications), falling back to the stock ISTA Ediabas BIN folder. Source: https://github.com/uholeschak/ediabaslib 2. ISTAVoltageControl.dll An external voltage-controller bridge that must be deployed into the folder resolved above before launching ISTA. This patch only installs the bootstrap call; all voltage logic (KL15/KL30 polling, VCI walk) lives inside that DLL. To implement your own, the expected contract is defined in PatchUtils.ISTAVoltage.cs (this repo): a public static method ISTAVoltageControl.Controller.StartFromIstaOperation(object serviceImpl). From there, use EdiabasVoltageBridge.TryRegister() (EdiabasLib) to receive KL15/KL30 samples via callback. If the flag is used but either dependency is missing, the silent try/catch ensures ISTA continues to start normally. * feat: add --enable-skip-battery-demand option (tautcony#89) * feat: block EDGE telemetry egress (Speedlink + OBFCM) in DataNotSend (tautcony#90) * feat: block EDGE telemetry egress (Speedlink + OBFCM) in DataNotSend The IsSend*Forbidden flags do not gate these channels: Logic.SendSpeedlinkDataInBackground and SendObfcmDataToVehicleShadowBackend fire the HTTP POST unconditionally (the forbidden flag only rides inside the payload), so the request still leaves the machine. Empty both senders (DnlibUtils.EmptyingMethod) so the telemetry call never happens. Scope: telemetry only -- Speedlink (session/dealer) + OBFCM (fuel data to Vehicle Shadow). EDGEBattery / EDGEPDI are left functional. To extend to zero EDGE egress, empty EDGEProcessorImpl.SendDataToBackend instead (single chokepoint, blocks all channels). * feat: block SCC vehicle-session upload in DataNotSend SaveCurrentSCCCase -> SCCProcessorImpl.PostVehicleSession runs unconditionally on session close and is not gated by the IsSend*Forbidden flags, posting VIN, vehicle data and diagnosis data (DTCs, technician statements, PDF files) to the SCC backend. Return its own offline sentinel (ServiceUnavailable) so the call never leaves the machine, mirroring PatchEdgeTelemetry. * feat: support legacy ICOM (A1/A2) over SLP on ISTA 4.60+ (tautcony#92) * feat: handle ICOM-Next device dispatch rewrite for ISTA 4.60+ ISTA 4.60 rewrote SLP device dispatch to only recognize DevType=="ICOM-Next"; legacy units reporting DevType=="ICOM" now fall through and get rejected ("newDevice is null or invalid"). Normalize DevType right after ParseAttrList on 4.60+ so the existing ICOM-Next dispatch branch is taken; keep the prior ReplaceDeviceType approach for versions below 4.60. * fix(PatchSLP): bind injected SLP dictionary members to the target corlib NormalizeDevType imported Dictionary.get_Item/set_Item via typeof(), which under the net10.0 patcher binds to System.Private.CoreLib. That assembly does not exist on ISTA's .NET Framework runtime, so the patched ScanDeviceFromAttrList failed to resolve at JIT and broke VCI detection (including genuine ICOM-Next). Reuse the existing mscorlib get_Item/op_Equality operands and build set_Item on the same declaring type. * feat(PatchSLP): A1/A2-aware ICOM dispatch with State gate (>= 4.60) Differentiating variant: instead of remapping DevType in the dictionary, widen the ICOM-Next dispatch condition to also accept DevType ICOM/ICOM A1/ICOM A2, mask their DevTypeExt to ICOM_Next_A, and force the raw reported State only for A1/A2. DevType is never rewritten, so A1/A2 stay distinguishable from a genuine ICOM-Next at the State block; genuine ICOM-Next keeps the upstream dispatch, firmware check and State intact (no firmware patch needed). Dictionary members are bound to the target corlib (mscorlib), not System.Private.CoreLib. * fix(PatchSLP): drive legacy ICOM off DevTypeExt, not DevType Real legacy ICOM A1/A2 report DevType="ICOM" with DevTypeExt="ICOM_A1"/"ICOM_A2"; only ICOM-Next reports DevType="ICOM-Next". The previous variant keyed on a non-existent DevType=="ICOM A1"/"ICOM A2" (dead branches) and overwrote DevTypeExt with "ICOM_Next_A", erasing the A1/A2 identity. Now: widen dispatch to also accept DevType=="ICOM"; leave DevTypeExt untouched; force raw State only when DevTypeExt is "ICOM_A1"/"ICOM_A2", so genuine ICOM-Next (DevTypeExt="ICOM_Next_A") keeps its upstream firmware/State. Reuses existing get_Item/ContainsKey/op_Equality operands. * refactor(PatchSLP): extract dispatch widening and State gate into named helpers Split the oversized RewriteIcomNextDispatch into WidenIcomDispatch and KeepRawStateForLegacyIcom, shrinking PatchSLP and addressing the CodeFactor complex-method report. Emitted IL is unchanged. * fix: scope the ENET real-ECU voltage-check patch to the diagnostic module loader only (tautcony#93) * feat: skip ENET real-ECU voltage check in ModuleBootstrapLoader.doIt() VoltageUtils.CheckVoltageForEthernetConnection (RheingoldProgramming.dll) reads real battery voltage via an ECU service program ("Batteriespannung") on ENET connections, independent of VCI.Kl15/Kl30Voltage. This means PatchISTAVoltageControl's injected voltage never covers it: an ENET session on battery alone can still show a genuine "battery voltage (terminal 30) below threshold value" message during plain diagnostic work, even while ISTA's header displays a healthy injected value. ModuleBootstrapLoader.doIt() -- the diagnostic test-module execution loop, invoked on every module load -- is one of three independent callers of that check. This patch targets only that call site: the leading (short-circuit &&) branch on ModuleLoader.IsModuleLoaderInitialized is turned into an unconditional jump to the same target the false-case already used, so the guarded block (including the voltage check call) is skipped regardless of the flag's real value. Logic.DoInitialElectricalChecks() (one-shot at connection) and the Toyota-only call inside ProgrammingUtils.CheckVehicleProgramingProhibits (a programming-plan gate) are deliberately left untouched -- this patch never affects anything reachable from a coding/flashing session. * docs: add missing <param>/<returns> tags (SA1611/SA1615) PatchModuleBootstrapLoaderEthernetVoltageCheck's XML doc comment was missing the <param name="module"> and <returns> tags flagged by CodeFactor/StyleCop. * feat: implement cancellation token support across commands and utilities * feat: reorganize test namespaces and add global usings for improved structure --------- Co-authored-by: TautCony <i@tautcony.xyz> Co-authored-by: OrpheeGT <OrpheeGT@users.noreply.github.com>
1 parent c8b5a33 commit e38ec28

77 files changed

Lines changed: 3813 additions & 1000 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

AGENTS.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,9 @@ dotnet build
4444
dotnet build -c Release
4545
dotnet test
4646
dotnet test src/ISTestA/ISTestA.csproj --filter FullyQualifiedName~UIAutomationTests
47-
bash scripts/check_copyright_years.sh
47+
python scripts/check_copyright_years.py
48+
python scripts/test-coverage.py
49+
python scripts/test-coverage.py --html --open
4850
```
4951

5052
Run the CLI:
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
## Approach
2+
3+
Use the existing `ISTgenerAtor` analyzer project to generate source from XSD files supplied as `AdditionalFiles` by `ISTAlter.csproj`. The XSD files become the canonical description of the XML entities. The generator maps the supported XSD subset used by these contracts to C# classes, enums, XML serialization attributes, and data contract attributes.
4+
5+
## Schema Scope
6+
7+
The generator supports the constructs required by the current models:
8+
9+
- top-level `xs:element`, `xs:complexType`, and `xs:simpleType`
10+
- ordered `xs:sequence` child elements
11+
- `xs:attribute` properties
12+
- inline simple enum restrictions
13+
- `maxOccurs="unbounded"` list properties
14+
- `nillable`, unqualified local elements, and XML data type annotations
15+
16+
## Handwritten Extensions
17+
18+
Behavior that cannot be represented by XSD remains in partial classes:
19+
20+
- `LicenseInfo.Clone()`
21+
- `DealerMasterData.Serialize<T>()`
22+
- constructors that initialize nested objects and lists
23+
- inheritance from `EntitySerializer<T>` for license entities
24+
25+
CLR-only generation hints are carried in `xs:annotation/xs:appinfo` under the
26+
`urn:ista-patcher:xsd-codegen` namespace. These annotations are intentionally
27+
outside the XML instance contract and use explicit names:
28+
29+
- `codegen:class`
30+
- `xsdType`
31+
- `clrBaseTypes`
32+
- `clrUsingNamespace`
33+
34+
## Tradeoffs
35+
36+
The generator intentionally supports the repository's XSD subset instead of becoming a general-purpose XSD-to-C# compiler. That keeps implementation small and deterministic while still moving the source of truth to XSD.
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
## Why
2+
3+
Rheingold XML model classes currently duplicate schema information in handwritten C# attributes. This makes XML contract changes error-prone because the schema and serializer attributes are not represented as first-class build inputs.
4+
5+
## What
6+
7+
- Add XSD files for the existing Rheingold license and dealer data XML contracts.
8+
- Generate the corresponding C# entity classes from those XSD files during compilation.
9+
- Keep non-schema behavior, such as cloning and serialization helpers, in handwritten partial classes.
10+
- Preserve existing public type names, namespaces, XML serialization attributes, and runtime behavior.
11+
12+
## Impact
13+
14+
- Affects `ISTAlter` XML model build inputs.
15+
- Extends `ISTgenerAtor` with XSD-driven model generation.
16+
- Existing code should continue using the same model type names.
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
## ADDED Requirements
2+
3+
### Requirement: XSD-backed Rheingold XML Models
4+
5+
Rheingold XML entity classes SHALL be generated from checked-in XSD schema files during compilation.
6+
7+
#### Scenario: License XML models are generated
8+
9+
- **GIVEN** the license schema is included in the project as a generator input
10+
- **WHEN** the project is compiled
11+
- **THEN** `LicenseInfo`, `LicensePackage`, and `LicenseType` are available with the existing XML serialization contract.
12+
13+
#### Scenario: Dealer data XML models are generated
14+
15+
- **GIVEN** the dealer data schema is included in the project as a generator input
16+
- **WHEN** the project is compiled
17+
- **THEN** dealer data classes and enums are available with the existing XML serialization contract.
18+
19+
#### Scenario: Non-schema behavior is preserved
20+
21+
- **GIVEN** generated XML model classes are used by existing code
22+
- **WHEN** callers clone licenses or serialize dealer master data
23+
- **THEN** those handwritten behaviors continue to work through partial class extensions.
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
## 1. Implementation
2+
3+
- [x] Add XSD files that represent the existing XML-root model contracts.
4+
- [x] Add an XSD source generator to `ISTgenerAtor`.
5+
- [x] Wire XSD files into `ISTAlter.csproj` as generator inputs.
6+
- [x] Replace handwritten XML data entities with generated partial classes and handwritten behavior extensions.
7+
8+
## 2. Verification
9+
10+
- [x] Add or update focused tests for generated XML model behavior.
11+
- [x] Run targeted tests for XML serialization.
12+
- [x] Run build or full tests as needed.
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
schema: spec-driven
2+
created: 2026-06-05
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
## Coverage Baseline
2+
3+
Baseline run for this change:
4+
5+
- Directory: `TestResults/20260605-203248`
6+
- Cobertura XML: `TestResults/20260605-203248/coverage.cobertura.xml`
7+
- HTML report: `TestResults/20260605-203248/coverage-report/index.html`
8+
- Overall line coverage: 27.6%
9+
- `ISTA-Patcher` line coverage: 0.0%
10+
- `ISTAvalon` line coverage: 67.4%
11+
- `ISTAlter` line coverage: 14.2%
12+
13+
This baseline is used to compare package and class coverage after the planned tests are implemented.
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
## Coverage Results
2+
3+
Baseline:
4+
5+
- Run: `TestResults/20260605-203248`
6+
- Overall lines: 27.67%
7+
- Overall branches: 27.86%
8+
- `ISTA-Patcher`: lines 0.00%, branches 0.00%
9+
- `ISTAvalon`: lines 67.43%, branches 58.24%
10+
- `ISTAlter`: lines 14.27%, branches 13.30%
11+
12+
Current:
13+
14+
- Run: `TestResults/20260605-205723`
15+
- Overall lines: 37.24%
16+
- Overall branches: 36.78%
17+
- `ISTA-Patcher`: lines 1.94%, branches 1.13%
18+
- `ISTAvalon`: lines 74.25%, branches 67.50%
19+
- `ISTAlter`: lines 26.71%, branches 23.38%
20+
21+
Largest targeted improvements:
22+
23+
- `ISTAPatcher.Utils.AvailablePorts`: 0% -> 100%
24+
- `ISTAvalon.Services.GuiObservationOptions`: 0% -> 100%
25+
- `ISTAvalon.Converters.LogMessageHighlighter`: 51.4% -> 85.6%
26+
- `ISTAlter.Models.Rheingold.LicenseManagement.LicenseStatusChecker`: 0% -> 100%
27+
- `ISTAlter.Utils.RegistryUtils`: 0% -> 95%
28+
- `ISTAlter.Utils.ResourceUtils`: 0% -> 57.1%
29+
- `ISTAlter.Core.PatchUtils`: 2.2% -> 11.7%
30+
31+
Remaining deferred targets:
32+
33+
- Real patch orchestration in `ISTAlter.Core.Patch`
34+
- High-complexity IL rewrites in `PatchUtils.Optional`
35+
- CLI command execution paths requiring real files, servers, or global process state
36+
- Platform/native utilities and telemetry initialization
Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
## Context
2+
3+
The latest coverage artifact is `TestResults/20260605-203248/coverage.cobertura.xml`. It reports:
4+
5+
- Overall line coverage: 27.6%
6+
- `ISTA-Patcher`: 0.0%
7+
- `ISTAvalon`: 67.4%
8+
- `ISTAlter`: 14.2%
9+
10+
The largest uncovered regions are not all equally useful targets. Some functions require real ISTA assemblies, platform-specific APIs, registry state, or Sentry/global process state. The coverage plan should prioritize deterministic tests that improve confidence without introducing flaky environment coupling.
11+
12+
## Goals / Non-Goals
13+
14+
**Goals:**
15+
16+
- Identify concrete functions that can raise coverage with stable, focused tests.
17+
- Prioritize tests by expected value and implementation risk.
18+
- Keep test changes in `src/ISTestA` unless a small test seam is needed.
19+
- Re-run `python scripts/test-coverage.py --html` and compare against `TestResults/20260605-203248`.
20+
21+
**Non-Goals:**
22+
23+
- Do not chase coverage by invoking real ISTA installations or patching unknown third-party binaries.
24+
- Do not require Windows registry, macOS Carbon/CoreFoundation, Linux machine IDs, or real network services for unit coverage.
25+
- Do not test Sentry delivery or external telemetry side effects.
26+
27+
## Decisions
28+
29+
### Prioritize Pure and Deterministic Functions First
30+
31+
P1 targets are pure or near-pure functions with high confidence and low setup cost:
32+
33+
- `ISTAvalon.Services.GuiObservationOptions.From`
34+
- Current coverage: 0/41 lines in `From`, 0/1 in `Prefix`, 0/3 in `ParsePort`, 0/5 in `IsTruthy`.
35+
- Test cases: env-only enablement, CLI enablement, falsey assignment, host via split and equals syntax, port via split and equals syntax, invalid port fallback, blank host fallback, `Prefix` formatting.
36+
- `ISTAvalon.Converters.LogMessageHighlighter.Highlight`
37+
- Current uncovered areas: non-ANSI quote/number path, empty messages, every level brush branch, ANSI reset/unknown/extended color edge cases.
38+
- Test cases: quoted strings strip quotes, numeric tokens use number brush, empty message returns one run, each `LogEventLevel` maps, ANSI 16-color/256-color/true-color/reset/invalid code behavior.
39+
- `ISTAPatcher.Utils.AvailablePorts.GetAvailablePort`
40+
- Current coverage: 0/13 lines.
41+
- Test cases: `startingPort > 65535` throws; starting from a valid high port returns a value in range. Avoid asserting a specific globally available port.
42+
43+
Alternative considered: start with large uncovered `PatchUtils.Optional` methods. Rejected for first phase because they need careful dnlib fixture assemblies and carry more risk.
44+
45+
### Add Focused Utility and Serialization Coverage Second
46+
47+
P2 targets require small fixtures but are still deterministic:
48+
49+
- `ISTAlter.Models.Rheingold.LicenseManagement.LicenseStatusChecker`
50+
- Current coverage: 0%.
51+
- Test cases: null `LicenseKey` returns false; generated key validates with matching RSA key; tampered license fails; deformatter creation uses expected hash algorithm path.
52+
- `ISTAlter.Utils.RegistryUtils.GenerateMockRegFile`
53+
- Current coverage: 0%.
54+
- Test cases: creates a `.reg` file without ISTA core DLL and defaults to native 64-bit hive; existing file is not overwritten when `force` is false; force overwrites. Assert generated text contains escaped license XML and `ForceDealerData`.
55+
- `ISTAlter.Utils.ResourceUtils`
56+
- Current coverage: 0%.
57+
- Test cases: update existing resource stream entry, preserve non-target entries, missing resource returns without throwing, missing file logs but preserves resource, `GetFromResource` returns target stream or throws when missing.
58+
59+
Alternative considered: cover `AddWatermark` immediately. Deferred because it uses SkiaSharp image decoding/fonts and should be a separate small image-fixture task after resource table coverage.
60+
61+
### Treat Patch and CLI Integration Paths as Third Phase
62+
63+
P3 targets are valuable but need test seams or generated fixture assemblies:
64+
65+
- `ISTAlter.Core.PatchUtils.Base`
66+
- Candidate functions: `HavePatchedMark`, `AddPatchedAttribute`, `SetPatchedMarkInner`, `IsVersionInRange`, `IsPatchApplicable`, `PatchFunction`, `PatchGetter`, `PatchAsyncFunction`.
67+
- Test strategy: build in-memory or temporary dnlib fixture assemblies with tiny types/methods and known attributes.
68+
- `ISTA-Patcher` commands and controllers
69+
- Current package coverage: 0%.
70+
- Test strategy: start with utility-level command dependencies (`AvailablePorts`), then add command tests only where file/network/console side effects can be isolated.
71+
- `TelemetryBootstrap.Initialize`
72+
- Current coverage: 0%.
73+
- Test strategy: either exclude from unit target or introduce a small test seam around Sentry/repository discovery before testing idempotence and tag fallback behavior.
74+
75+
## Risks / Trade-offs
76+
77+
- [Risk] Tests that assert exact available ports can be flaky on busy machines. → Mitigation: assert range and validity, not exact port, except for invalid input.
78+
- [Risk] Environment variable tests can leak process state. → Mitigation: save and restore `ISTA_GUI_DUMP_HTTP*` variables in setup/teardown.
79+
- [Risk] RSA/XML signature tests can become brittle if serialization changes intentionally. → Mitigation: assert round-trip validity and tamper failure, not fixed signature bytes.
80+
- [Risk] dnlib patch tests can overfit implementation details. → Mitigation: test public helper outcomes and assembly metadata changes before testing specific IL rewrites.
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
## P3 Evaluation
2+
3+
### Patch Function Helpers
4+
5+
`PatchFunction` and `PatchGetter` can be covered with temporary dnlib metadata fixtures. The implementation now accepts `ModuleDef`, which matches the members actually used by the helpers and allows `ModuleDefUser` in-memory tests. `PatchAsyncFunction` is partially covered through the missing-method branch.
6+
7+
Full positive-path `PatchAsyncFunction` coverage is deferred because a faithful async state-machine fixture requires constructing an `AsyncStateMachineAttribute`, nested generated type, override metadata, and a valid `MoveNext` body. That setup risks overfitting dnlib metadata details instead of testing stable behavior.
8+
9+
### Telemetry Bootstrap
10+
11+
`TelemetryBootstrap.Initialize` should remain out of the immediate unit coverage target. It initializes global Sentry state, reads git repository metadata, and is intentionally process-global/idempotent. Covering it well would require a seam around Sentry initialization and repository discovery. That seam is reasonable only if telemetry behavior changes.
12+
13+
### CLI Command Coverage
14+
15+
CLI command coverage should start with paths that avoid real ISTA files, live HTTP servers, global console state, or Sentry side effects:
16+
17+
- Utility-level dependencies such as `AvailablePorts`.
18+
- Argument-to-model parsing where command descriptors can be constructed without executing patch operations.
19+
- File-generation commands only when all file system effects are isolated under temporary directories.
20+
21+
Patch command execution, server startup, crypto file-list processing, and commands requiring real input files should remain integration-test candidates.

0 commit comments

Comments
 (0)