tests: Added unit tests for new key parts of Gamenative#1143
Open
unbelievableflavour wants to merge 4 commits intoutkarshdalal:masterfrom
Open
tests: Added unit tests for new key parts of Gamenative#1143unbelievableflavour wants to merge 4 commits intoutkarshdalal:masterfrom
unbelievableflavour wants to merge 4 commits intoutkarshdalal:masterfrom
Conversation
unbelievableflavour
changed the title
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced direct static-collection access with injectable provider functions in three production modules and added internal test-only setters to override them; added multiple Robolectric/MockK tests covering game fixes, launch-dependencies, and pre-install steps (markers, command building, downloads, and registry edits). Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
app/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt (1)
47-47: Remove redundantcontainer.drivesstubbing in this test.
VcRedistStep.buildCommand(...)is file-based viagameDir, so this stub adds noise without affecting behavior.♻️ Proposed cleanup
- every { container.drives } returns "A:${gameDir.absolutePath}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt` at line 47, Remove the redundant stubbing of container.drives in the VcRedistStepTest: delete the line that does every { container.drives } returns "A:${gameDir.absolutePath}" since VcRedistStep.buildCommand(...) derives behavior from the gameDir file path and this stub is unnecessary noise; keep other mocks intact and run the test to confirm no behavior change.app/src/test/java/app/gamenative/utils/launchdependencies/GogScriptInterpreterDependencyTest.kt (2)
65-72: Temporary directories are not cleaned up after tests.The test creates multiple temporary directories via
createTempDirectory()(lines 65, 76, 89, 136) but never deletes them. While the JVM may eventually clean these up, it's better practice to explicitly delete them in@Afteror use JUnit's@TempDirrule.♻️ Suggested improvement using `@TempDir`
+import org.junit.Rule +import org.junit.rules.TemporaryFolder `@RunWith`(RobolectricTestRunner::class) class GogScriptInterpreterDependencyTest { private lateinit var context: Context private lateinit var container: Container + + `@get`:Rule + val tempFolder = TemporaryFolder()Then replace
createTempDirectory(prefix = "...").toFile()calls withtempFolder.newFolder("...").Also applies to: 76-85, 89-132, 136-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/launchdependencies/GogScriptInterpreterDependencyTest.kt` around lines 65 - 72, The tests in GogScriptInterpreterDependencyTest create temporary directories via createTempDirectory() (variables like installDir) and never clean them up; replace those manual temp directories by using JUnit's `@TempDir` (declare a TempDir field) and use that temp directory for the test folders (or alternatively delete the created directories in an `@After` method), updating the test cases that call createTempDirectory() (the instances around the installDir and other temp folder creations) to use the shared `@TempDir-managed` directory so cleanup is handled automatically.
147-149: Consider a more meaningful assertion for graceful exit.
assertTrue(true)is a no-op assertion that provides no verification. While the comment explains the intent (reaching this point means graceful exit), the test would pass even if an exception were thrown and caught elsewhere.Consider verifying that no unexpected interactions occurred:
♻️ Alternative assertion
- // Reaching this point means it exited gracefully. - assertTrue(true) + // Verify install did not attempt any download when service is unavailable + coVerify(exactly = 0) { any<GOGDownloadManager>().downloadDependenciesWithProgress(any(), any(), any(), any(), any()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/launchdependencies/GogScriptInterpreterDependencyTest.kt` around lines 147 - 149, The final no-op assertion in GogScriptInterpreterDependencyTest (assertTrue(true)) should be replaced with a meaningful check that the interpreter exited gracefully: wrap the invocation under test in an assertion like assertDoesNotThrow { /* call the method that runs the interpreter */ } and/or assert expected mock interactions (e.g., verifyNoMoreInteractions(mockInstaller) or verify(mockLogger).info(...)) to confirm no unexpected behavior; update the test in class GogScriptInterpreterDependencyTest to call assertDoesNotThrow around the interpreter run and add verifyNoMoreInteractions/verify calls for the relevant mocks used in the test.app/src/test/java/app/gamenative/utils/PreInstallStepsTest.kt (1)
39-50: Temporary directory is not cleaned up after tests.Similar to
GogScriptInterpreterDependencyTest, thegameDirtemp directory created insetUp()is never deleted. Consider using JUnit'sTemporaryFolderrule for automatic cleanup.♻️ Suggested improvement
+import org.junit.Rule +import org.junit.rules.TemporaryFolder `@RunWith`(RobolectricTestRunner::class) class PreInstallStepsTest { private lateinit var container: Container - private lateinit var gameDir: File + + `@get`:Rule + val tempFolder = TemporaryFolder() + + private val gameDir: File + get() = tempFolder.root `@Before` fun setUp() { container = mockk(relaxed = true) - gameDir = createTempDirectory(prefix = "preinstall-steps-test").toFile() every { container.drives } returns "A:${gameDir.absolutePath}" every { container.containerVariant } returns Container.BIONIC }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/PreInstallStepsTest.kt` around lines 39 - 50, The test creates a temp directory via createTempDirectory() assigned to gameDir in setUp() but never deletes it; update tearDown() to remove the temp directory (gameDir) after tests (or replace createTempDirectory usage with JUnit TemporaryFolder/TempDir rule) so the filesystem is cleaned; modify the existing tearDown() (which currently calls PreInstallSteps.setStepsProviderForTests(null)) to also delete gameDir (or switch setUp/tearDown to use the TemporaryFolder/TempDir mechanism) and ensure any mocking of container.drives still points to the cleaned/managed directory.app/src/test/java/app/gamenative/utils/LaunchDependenciesTest.kt (1)
138-171: Consider verifying the exception was actually thrown.The test catches
IllegalStateExceptionbut doesn't assert that the exception was thrown. If the production code silently swallows the exception, this test would still pass.♻️ Suggested improvement
+ var exceptionThrown = false try { sut.ensureLaunchDependencies( context = context, container = container, gameSource = GameSource.GOG, gameId = 9, setLoadingMessage = { messages += it }, setLoadingProgress = { progress += it }, ) } catch (_: IllegalStateException) { - // Expected in this test. + exceptionThrown = true } + assertTrue("Expected IllegalStateException to be thrown", exceptionThrown) assertEquals(context.getString(R.string.main_loading), messages.last()) assertEquals(1f, progress.last())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/LaunchDependenciesTest.kt` around lines 138 - 171, The test ensureLaunchDependencies_resetsLoadingUi_evenIfInstallFails currently catches IllegalStateException without asserting it was thrown; change the test to explicitly assert the exception from sut.ensureLaunchDependencies (for example using assertFailsWith<IllegalStateException> or capturing a Boolean/flag) while keeping the existing checks on messages.last() and progress.last(), and reference the failing call to GogScriptInterpreterDependency.install invoked via sut.ensureLaunchDependencies to ensure the install exception is actually propagated.app/src/test/java/app/gamenative/gamefixes/LaunchArgFixTest.kt (1)
43-53: Add whitespace-onlyexecArgscoverage.
LaunchArgFix.apply(...)usestrim(). A" "input should be treated as empty and apply the fix, but that path is not currently tested.Suggested additional test
+ `@Test` + fun apply_setsLaunchArgs_whenContainerArgsAreWhitespaceOnly() { + val fix = LaunchArgFix("-lang=eng") + every { container.execArgs } returns " " + every { container.execArgs = any() } just runs + every { container.saveData() } just runs + + val result = fix.apply(context, "1129934535", "/tmp/game", "A:\\", container) + + assertTrue(result) + verify(exactly = 1) { container.execArgs = "-lang=eng" } + verify(exactly = 1) { container.saveData() } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/gamefixes/LaunchArgFixTest.kt` around lines 43 - 53, Add a new unit test in LaunchArgFixTest that covers whitespace-only execArgs by creating a test (e.g., apply_applies_whenContainerExecArgsIsWhitespace) that constructs LaunchArgFix("-lang=eng"), mocks container.execArgs to return a string of only spaces (e.g., " "), calls fix.apply(context, "1129934535", "/tmp/game", "A:\\", container), asserts the result is true, and verifies that container.execArgs is assigned (and container.saveData() is invoked) — this verifies LaunchArgFix.apply treats trim()-only whitespace as empty and applies the fix.app/src/main/java/app/gamenative/gamefixes/GameFixesRegistry.kt (1)
41-41: Thread-safety approach should match practical risk profile. WhilefixesProvideris technically mutable shared state without synchronization, it's a test-only hook with controlled access timing (explicit@Before/@Afterreset) and runs single-threaded (RobolectricTestRunner). AnAtomicReferencewrapper may be unnecessary over-engineering. If this remains a concern, consider making the provider immutable at construction time or gating test overrides with a test-time-only guard instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/gamefixes/GameFixesRegistry.kt` at line 41, The mutable shared var fixesProvider currently allows test-only overrides without synchronization (symbol: fixesProvider) which the reviewer flagged as overkill for thread-safety; either make the provider immutable by initializing fixesProvider at construction time from the existing fixes map (use the constructor to set fixesProvider = { fixes }) or add a test-only guard that only permits overrides under a DEBUG/TEST flag (expose a setOverridesForTests(...) helper) so production usage cannot mutate fixesProvider at runtime; update usages that reset it in tests to call the new test-only helper or rely on the immutable provider (refer to fixesProvider and fixes to locate changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/test/java/app/gamenative/gamefixes/GameFixesRegistryTest.kt`:
- Around line 35-39: The tearDown() currently calls unmockkAll(), which clears
global mock state; instead only unmock the specific global mock
GOGService.Companion. Replace the unmockkAll() call in the tearDown() method
with unmockkObject(GOGService.Companion) and keep the existing
GameFixesRegistry.setFixesProviderForTests(null) call so only the required
global mock is cleaned up.
In `@app/src/test/java/app/gamenative/gamefixes/GOGDependencyFixTest.kt`:
- Around line 38-41: Replace the broad unmockkAll() call in tearDown() with
targeted unmockkObject(GOGService.Companion) to only undo the mocked companion
object; locate the tearDown() method in GOGDependencyFixTest and change the
cleanup to call unmockkObject on GOGService.Companion so other mocks in the test
suite aren’t impacted during parallel runs.
In `@app/src/test/java/app/gamenative/gamefixes/RegistryKeyFixTest.kt`:
- Around line 29-35: Tests like apply_returnsFalse_whenSystemRegDoesNotExist
mutate the shared Wine prefix file (system.reg) causing cross-test
contamination; modify the tests to isolate changes by creating a temporary
wineprefix or backing up and restoring the registry file: for the test that uses
ImageFs.find(context) and the local systemRegFile variable, either point ImageFs
to a temp directory (so system.reg is created/removed in a per-test temp prefix)
or copy the existing system.reg to a backup before deleting/modifying and
restore it in a teardown/@After block (or finally) to ensure original content is
always restored; apply the same pattern to the other tests referenced (lines
~49-55 and ~75-83) that manipulate system.reg.
In
`@app/src/test/java/app/gamenative/utils/preInstallSteps/GogScriptInterpreterStepTest.kt`:
- Around line 41-49: The test method
appliesTo_requiresGogAndGlibcAndMissingMarker should also verify the "marker
present" branch: after asserting appliesTo is true for Container.GLIBC and
GameSource.GOG, create or mock the presence of Marker.GOG_SCRIPT_INSTALLED in
the gameDir (so the marker check inside GogScriptInterpreterStep.appliesTo sees
the marker) and assert that GogScriptInterpreterStep.appliesTo(container,
GameSource.GOG, gameDir.absolutePath) then returns false; ensure you clean
up/remove the marker or reset the mock before the later variant assertions
(e.g., before switching container to Container.BIONIC).
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/gamefixes/GameFixesRegistry.kt`:
- Line 41: The mutable shared var fixesProvider currently allows test-only
overrides without synchronization (symbol: fixesProvider) which the reviewer
flagged as overkill for thread-safety; either make the provider immutable by
initializing fixesProvider at construction time from the existing fixes map (use
the constructor to set fixesProvider = { fixes }) or add a test-only guard that
only permits overrides under a DEBUG/TEST flag (expose a
setOverridesForTests(...) helper) so production usage cannot mutate
fixesProvider at runtime; update usages that reset it in tests to call the new
test-only helper or rely on the immutable provider (refer to fixesProvider and
fixes to locate changes).
In `@app/src/test/java/app/gamenative/gamefixes/LaunchArgFixTest.kt`:
- Around line 43-53: Add a new unit test in LaunchArgFixTest that covers
whitespace-only execArgs by creating a test (e.g.,
apply_applies_whenContainerExecArgsIsWhitespace) that constructs
LaunchArgFix("-lang=eng"), mocks container.execArgs to return a string of only
spaces (e.g., " "), calls fix.apply(context, "1129934535", "/tmp/game",
"A:\\", container), asserts the result is true, and verifies that
container.execArgs is assigned (and container.saveData() is invoked) — this
verifies LaunchArgFix.apply treats trim()-only whitespace as empty and applies
the fix.
In
`@app/src/test/java/app/gamenative/utils/launchdependencies/GogScriptInterpreterDependencyTest.kt`:
- Around line 65-72: The tests in GogScriptInterpreterDependencyTest create
temporary directories via createTempDirectory() (variables like installDir) and
never clean them up; replace those manual temp directories by using JUnit's
`@TempDir` (declare a TempDir field) and use that temp directory for the test
folders (or alternatively delete the created directories in an `@After` method),
updating the test cases that call createTempDirectory() (the instances around
the installDir and other temp folder creations) to use the shared
`@TempDir-managed` directory so cleanup is handled automatically.
- Around line 147-149: The final no-op assertion in
GogScriptInterpreterDependencyTest (assertTrue(true)) should be replaced with a
meaningful check that the interpreter exited gracefully: wrap the invocation
under test in an assertion like assertDoesNotThrow { /* call the method that
runs the interpreter */ } and/or assert expected mock interactions (e.g.,
verifyNoMoreInteractions(mockInstaller) or verify(mockLogger).info(...)) to
confirm no unexpected behavior; update the test in class
GogScriptInterpreterDependencyTest to call assertDoesNotThrow around the
interpreter run and add verifyNoMoreInteractions/verify calls for the relevant
mocks used in the test.
In `@app/src/test/java/app/gamenative/utils/LaunchDependenciesTest.kt`:
- Around line 138-171: The test
ensureLaunchDependencies_resetsLoadingUi_evenIfInstallFails currently catches
IllegalStateException without asserting it was thrown; change the test to
explicitly assert the exception from sut.ensureLaunchDependencies (for example
using assertFailsWith<IllegalStateException> or capturing a Boolean/flag) while
keeping the existing checks on messages.last() and progress.last(), and
reference the failing call to GogScriptInterpreterDependency.install invoked via
sut.ensureLaunchDependencies to ensure the install exception is actually
propagated.
In `@app/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt`:
- Line 47: Remove the redundant stubbing of container.drives in the
VcRedistStepTest: delete the line that does every { container.drives } returns
"A:${gameDir.absolutePath}" since VcRedistStep.buildCommand(...) derives
behavior from the gameDir file path and this stub is unnecessary noise; keep
other mocks intact and run the test to confirm no behavior change.
In `@app/src/test/java/app/gamenative/utils/PreInstallStepsTest.kt`:
- Around line 39-50: The test creates a temp directory via createTempDirectory()
assigned to gameDir in setUp() but never deletes it; update tearDown() to remove
the temp directory (gameDir) after tests (or replace createTempDirectory usage
with JUnit TemporaryFolder/TempDir rule) so the filesystem is cleaned; modify
the existing tearDown() (which currently calls
PreInstallSteps.setStepsProviderForTests(null)) to also delete gameDir (or
switch setUp/tearDown to use the TemporaryFolder/TempDir mechanism) and ensure
any mocking of container.drives still points to the cleaned/managed directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e0281df-fab9-418c-bb0b-5977c8fc5fe3
📒 Files selected for processing (17)
app/src/main/java/app/gamenative/gamefixes/GameFixesRegistry.ktapp/src/main/java/app/gamenative/utils/LaunchDependencies.ktapp/src/main/java/app/gamenative/utils/PreInstallSteps.ktapp/src/test/java/app/gamenative/gamefixes/GOGDependencyFixTest.ktapp/src/test/java/app/gamenative/gamefixes/GameFixesRegistryTest.ktapp/src/test/java/app/gamenative/gamefixes/KeyedGameFixTypesTest.ktapp/src/test/java/app/gamenative/gamefixes/LaunchArgFixTest.ktapp/src/test/java/app/gamenative/gamefixes/RegistryKeyFixTest.ktapp/src/test/java/app/gamenative/utils/LaunchDependenciesTest.ktapp/src/test/java/app/gamenative/utils/PreInstallStepsTest.ktapp/src/test/java/app/gamenative/utils/launchdependencies/GogScriptInterpreterDependencyTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/GogScriptInterpreterStepTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/OpenALStepTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/PhysXStepTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/UbisoftConnectStepTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/XnaFrameworkStepTest.kt
app/src/test/java/app/gamenative/gamefixes/types/GOGDependencyFixTest.kt
Show resolved
Hide resolved
app/src/test/java/app/gamenative/utils/preInstallSteps/GogScriptInterpreterStepTest.kt
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/test/java/app/gamenative/utils/LaunchDependenciesTest.kt`:
- Around line 62-63: Tests like
getLaunchDependencies_returnsEmpty_whenDependencyDoesNotApply call
sut.getLaunchDependencies but still use the production LaunchDependency list,
making them brittle; update these tests to inject a controlled dependencies
provider into sut (either via the constructor or the available setter like
setDependenciesProvider or provideDependencies) before calling
sut.getLaunchDependencies so each test uses an explicit, minimal list (e.g.,
empty or one-mock LaunchDependency) rather than the global production list;
apply the same change to the other tests referenced (the ones around lines 69,
110, 140) so all behavior tests are list-independent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 073f6333-6e1a-4434-aea3-b4f9750dd574
📒 Files selected for processing (7)
app/src/test/java/app/gamenative/gamefixes/LaunchArgFixTest.ktapp/src/test/java/app/gamenative/gamefixes/RegistryKeyFixTest.ktapp/src/test/java/app/gamenative/utils/LaunchDependenciesTest.ktapp/src/test/java/app/gamenative/utils/PreInstallStepsTest.ktapp/src/test/java/app/gamenative/utils/launchdependencies/GogScriptInterpreterDependencyTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/GogScriptInterpreterStepTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt
🚧 Files skipped from review as they are similar to previous changes (6)
- app/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt
- app/src/test/java/app/gamenative/gamefixes/LaunchArgFixTest.kt
- app/src/test/java/app/gamenative/utils/PreInstallStepsTest.kt
- app/src/test/java/app/gamenative/gamefixes/RegistryKeyFixTest.kt
- app/src/test/java/app/gamenative/utils/launchdependencies/GogScriptInterpreterDependencyTest.kt
- app/src/test/java/app/gamenative/utils/preInstallSteps/GogScriptInterpreterStepTest.kt
unbelievableflavour
force-pushed
the
more-unit-tests
branch
from
99e24ed to
6a422a0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/src/test/java/app/gamenative/utils/PreInstallStepsTest.kt (1)
122-135: Consider documenting marker list sync requirement.The
expectedMarkerslist is a snapshot ofPreInstallSteps.steps.map { it.marker }. If new steps are added to production code, this test will pass but under-assert. A brief comment acknowledging this trade-off (per PR objectives) would help future maintainers.📝 Suggested documentation
`@Test` fun markAllDone_createsAllPreInstallMarkerFiles() { PreInstallSteps.markAllDone(container) + // Note: This list mirrors the production steps list. If steps are added, + // update this list to maintain full coverage. val expectedMarkers = listOf( Marker.VCREDIST_INSTALLED,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/PreInstallStepsTest.kt` around lines 122 - 135, The test markAllDone_createsAllPreInstallMarkerFiles uses a hard-coded expectedMarkers list that is a snapshot of PreInstallSteps.steps.map { it.marker }; add a brief inline comment above the expectedMarkers declaration explaining this snapshot must be kept in sync with PreInstallSteps.steps (or updated if new steps are added) so future maintainers understand the trade-off and why the list is explicit rather than derived from PreInstallSteps.steps.app/src/test/java/app/gamenative/utils/LaunchDependenciesTest.kt (1)
144-181: Consider usingassertThrowsfor cleaner exception assertion.The try-catch pattern works but
assertThrowsis more idiomatic and concise.♻️ Alternative using assertThrows
`@Test` fun ensureLaunchDependencies_resetsLoadingUi_evenIfInstallFails() = runBlocking { // ... setup ... val messages = mutableListOf<String>() val progress = mutableListOf<Float>() assertThrows(IllegalStateException::class.java) { runBlocking { sut.ensureLaunchDependencies( context = context, container = container, gameSource = GameSource.GOG, gameId = 9, setLoadingMessage = { messages += it }, setLoadingProgress = { progress += it }, ) } } assertEquals(context.getString(R.string.main_loading), messages.last()) assertEquals(1f, progress.last()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/LaunchDependenciesTest.kt` around lines 144 - 181, Replace the manual try/catch/flag pattern in the test ensureLaunchDependencies_resetsLoadingUi_evenIfInstallFails with JUnit's assertThrows to make the exception assertion concise and idiomatic: call assertThrows(IllegalStateException::class.java) and inside its lambda invoke sut.ensureLaunchDependencies(...) (wrapping in runBlocking if needed), then keep the existing checks on messages.last() and progress.last(); this targets the sut.ensureLaunchDependencies invocation and preserves the setup for GogScriptInterpreterDependency and the message/progress collections.app/src/test/java/app/gamenative/utils/preInstallSteps/GogScriptInterpreterStepTest.kt (1)
36-39: Temp directory not cleaned up in tearDown.The
gameDiris created viacreateTempDirectory()but never deleted. While temp directories are eventually cleaned by the OS, explicit cleanup ensures test isolation and avoids accumulating test artifacts.♻️ Suggested fix
`@After` fun tearDown() { unmockkAll() + gameDir.deleteRecursively() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/preInstallSteps/GogScriptInterpreterStepTest.kt` around lines 36 - 39, The tearDown method in GogScriptInterpreterStepTest currently only calls unmockkAll() but doesn't delete the temporary directory created by createTempDirectory() (gameDir); update tearDown to safely delete gameDir (e.g., if gameDir is a Path/File use gameDir.toFile().deleteRecursively() or a Files.walkFileTree cleanup) with a null/existence check and optional try/catch to avoid hiding test failures, then call unmockkAll(); ensure you reference the class variable gameDir and keep tearDown annotated `@After`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/test/java/app/gamenative/utils/PreInstallStepsTest.kt`:
- Around line 23-37: The test file declares FakeStep implementing PreInstallStep
but lacks the import for the PreInstallStep interface; add the missing import
for PreInstallStep at the top of app/src/test/java/.../PreInstallStepsTest.kt so
the FakeStep declaration (FakeStep, PreInstallStep) compiles, then re-run the
tests to confirm compilation passes.
---
Nitpick comments:
In `@app/src/test/java/app/gamenative/utils/LaunchDependenciesTest.kt`:
- Around line 144-181: Replace the manual try/catch/flag pattern in the test
ensureLaunchDependencies_resetsLoadingUi_evenIfInstallFails with JUnit's
assertThrows to make the exception assertion concise and idiomatic: call
assertThrows(IllegalStateException::class.java) and inside its lambda invoke
sut.ensureLaunchDependencies(...) (wrapping in runBlocking if needed), then keep
the existing checks on messages.last() and progress.last(); this targets the
sut.ensureLaunchDependencies invocation and preserves the setup for
GogScriptInterpreterDependency and the message/progress collections.
In
`@app/src/test/java/app/gamenative/utils/preInstallSteps/GogScriptInterpreterStepTest.kt`:
- Around line 36-39: The tearDown method in GogScriptInterpreterStepTest
currently only calls unmockkAll() but doesn't delete the temporary directory
created by createTempDirectory() (gameDir); update tearDown to safely delete
gameDir (e.g., if gameDir is a Path/File use
gameDir.toFile().deleteRecursively() or a Files.walkFileTree cleanup) with a
null/existence check and optional try/catch to avoid hiding test failures, then
call unmockkAll(); ensure you reference the class variable gameDir and keep
tearDown annotated `@After`.
In `@app/src/test/java/app/gamenative/utils/PreInstallStepsTest.kt`:
- Around line 122-135: The test markAllDone_createsAllPreInstallMarkerFiles uses
a hard-coded expectedMarkers list that is a snapshot of
PreInstallSteps.steps.map { it.marker }; add a brief inline comment above the
expectedMarkers declaration explaining this snapshot must be kept in sync with
PreInstallSteps.steps (or updated if new steps are added) so future maintainers
understand the trade-off and why the list is explicit rather than derived from
PreInstallSteps.steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 899eb86a-6b2d-4854-a52a-2399a944008b
📒 Files selected for processing (7)
app/src/test/java/app/gamenative/gamefixes/LaunchArgFixTest.ktapp/src/test/java/app/gamenative/gamefixes/RegistryKeyFixTest.ktapp/src/test/java/app/gamenative/utils/LaunchDependenciesTest.ktapp/src/test/java/app/gamenative/utils/PreInstallStepsTest.ktapp/src/test/java/app/gamenative/utils/launchdependencies/GogScriptInterpreterDependencyTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/GogScriptInterpreterStepTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/test/java/app/gamenative/gamefixes/RegistryKeyFixTest.kt
- app/src/test/java/app/gamenative/gamefixes/LaunchArgFixTest.kt
- app/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt
unbelievableflavour
changed the title
unbelievableflavour
force-pushed
the
more-unit-tests
branch
from
6a422a0 to
d2a9884
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/test/java/app/gamenative/utils/LaunchDependenciesTest.kt (1)
144-182: LGTM with optional style improvement.This test correctly verifies that the
finallyblock resets the loading UI even wheninstall()throws an exception—important coverage for the try-finally guarantee.Minor style nit: Line 178 could use
assertNotNull(thrown)instead ofassertTrue(thrown != null)for more idiomatic JUnit assertions.♻️ Optional style improvement
- assertTrue(thrown != null) + assertNotNull(thrown)You'll need to add the import:
import org.junit.Assert.assertNotNull🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/LaunchDependenciesTest.kt` around lines 144 - 182, Replace the non-idiomatic null check in the test ensureLaunchDependencies_resetsLoadingUi_evenIfInstallFails by using assertNotNull(thrown) instead of assertTrue(thrown != null), and add the corresponding import org.junit.Assert.assertNotNull so the assertion compiles; keep the rest of the test (variables thrown, messages, progress, and final assertions) unchanged.app/src/test/java/app/gamenative/utils/preInstallSteps/GogScriptInterpreterStepTest.kt (1)
36-39: Consider adding temp directory cleanup for consistency.The
tearDownmethod unmocks objects but doesn't clean up the temporary directory created insetUp. For consistency withPreInstallStepsTest.kt, consider addinggameDir.deleteRecursively().♻️ Suggested addition
`@After` fun tearDown() { unmockkAll() + gameDir.deleteRecursively() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/preInstallSteps/GogScriptInterpreterStepTest.kt` around lines 36 - 39, The tearDown() currently only calls unmockkAll() but doesn't remove the temporary gameDir created in setUp; update the GogScriptInterpreterStepTest.tearDown method to also clean up the temp directory by invoking gameDir.deleteRecursively() (call it before or after unmockkAll() as appropriate) so the temporary files are removed consistently with PreInstallStepsTest.app/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt (1)
23-27: Consider adding cleanup for the temporary directory.The test creates a temporary directory in
setUpbut doesn't clean it up. WhilecreateTempDirectoryfiles are eventually removed by the OS, explicit cleanup improves test hygiene and matches the pattern used inPreInstallStepsTest.kt.♻️ Suggested cleanup addition
+ `@After` + fun tearDown() { + gameDir.deleteRecursively() + } + `@Test` fun appliesTo_returnsTrue_whenMarkerMissing() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt` around lines 23 - 27, Add explicit cleanup for the temp directory created in setUp(): ensure the VcRedistStepTest class removes gameDir after each test by adding an `@After` teardown method that checks gameDir for null and deletes it recursively (mirror the pattern used in PreInstallStepsTest.kt); reference the setUp() method, the gameDir property and the createTempDirectory call when locating where to add the teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/test/java/app/gamenative/utils/LaunchDependenciesTest.kt`:
- Around line 144-182: Replace the non-idiomatic null check in the test
ensureLaunchDependencies_resetsLoadingUi_evenIfInstallFails by using
assertNotNull(thrown) instead of assertTrue(thrown != null), and add the
corresponding import org.junit.Assert.assertNotNull so the assertion compiles;
keep the rest of the test (variables thrown, messages, progress, and final
assertions) unchanged.
In
`@app/src/test/java/app/gamenative/utils/preInstallSteps/GogScriptInterpreterStepTest.kt`:
- Around line 36-39: The tearDown() currently only calls unmockkAll() but
doesn't remove the temporary gameDir created in setUp; update the
GogScriptInterpreterStepTest.tearDown method to also clean up the temp directory
by invoking gameDir.deleteRecursively() (call it before or after unmockkAll() as
appropriate) so the temporary files are removed consistently with
PreInstallStepsTest.
In `@app/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt`:
- Around line 23-27: Add explicit cleanup for the temp directory created in
setUp(): ensure the VcRedistStepTest class removes gameDir after each test by
adding an `@After` teardown method that checks gameDir for null and deletes it
recursively (mirror the pattern used in PreInstallStepsTest.kt); reference the
setUp() method, the gameDir property and the createTempDirectory call when
locating where to add the teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 023ee1fa-4c1e-49a9-bae2-072f0271f107
📒 Files selected for processing (7)
app/src/test/java/app/gamenative/gamefixes/LaunchArgFixTest.ktapp/src/test/java/app/gamenative/gamefixes/RegistryKeyFixTest.ktapp/src/test/java/app/gamenative/utils/LaunchDependenciesTest.ktapp/src/test/java/app/gamenative/utils/PreInstallStepsTest.ktapp/src/test/java/app/gamenative/utils/launchdependencies/GogScriptInterpreterDependencyTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/GogScriptInterpreterStepTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt
✅ Files skipped from review due to trivial changes (1)
- app/src/test/java/app/gamenative/utils/launchdependencies/GogScriptInterpreterDependencyTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/test/java/app/gamenative/gamefixes/RegistryKeyFixTest.kt
- app/src/test/java/app/gamenative/gamefixes/LaunchArgFixTest.kt
unbelievableflavour
changed the title
unbelievableflavour
force-pushed
the
more-unit-tests
branch
from
d2a9884 to
5a65f01
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/test/java/app/gamenative/gamefixes/GOGDependencyFixTest.kt (1)
38-41:⚠️ Potential issue | 🟠 MajorUse scoped MockK cleanup in
tearDown()instead of global reset.
unmockkAll()on Line 40 clears global MockK state and can interfere with other tests. Here, onlyGOGService.Companionis object-mocked (Line 35), so cleanup should be targeted.Suggested fix
-import io.mockk.unmockkAll +import io.mockk.unmockkObject @@ `@After` fun tearDown() { - unmockkAll() + unmockkObject(GOGService.Companion) }#!/bin/bash # Verify this test class only object-mocks GOGService.Companion and uses broad cleanup. rg -n --type kotlin -C2 '\bmockkObject\s*\(|\bunmockk(All|Object)\s*\(' app/src/test/java/app/gamenative/gamefixes/GOGDependencyFixTest.ktExpected result: one
mockkObject(GOGService.Companion)andunmockkAll()currently intearDown(); after fix,unmockkObject(GOGService.Companion).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/gamefixes/GOGDependencyFixTest.kt` around lines 38 - 41, The teardown uses a global MockK cleanup (unmockkAll()) which can affect other tests; replace it with a scoped cleanup by calling unmockkObject(GOGService.Companion) in the tearDown() of GOGDependencyFixTest to match the mockkObject(GOGService.Companion) used in the test setup (refer to the tearDown function and the mockkObject call for GOGService.Companion).
🧹 Nitpick comments (2)
app/src/test/java/app/gamenative/utils/preInstallSteps/OpenALStepTest.kt (1)
22-26: Clean up temp directories after each test.Line 25 creates a new temp directory per test, but there’s no teardown cleanup. Add an
@Afterhook to prevent temp-dir buildup across repeated runs.♻️ Proposed fix
import org.junit.Before +import org.junit.After import org.junit.Test @@ class OpenALStepTest { @@ `@Before` fun setUp() { container = mockk(relaxed = true) gameDir = createTempDirectory(prefix = "openal-step-test").toFile() } + + `@After` + fun tearDown() { + if (::gameDir.isInitialized) { + gameDir.deleteRecursively() + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/preInstallSteps/OpenALStepTest.kt` around lines 22 - 26, Tests create a temporary directory in setUp (gameDir in OpenALStepTest) but never remove it; add an `@After` annotated teardown method (e.g., tearDown) that deletes the temp directory (call gameDir.deleteRecursively() or equivalent) to clean up after each test and prevent temp-dir buildup across runs.app/src/test/java/app/gamenative/utils/PreInstallStepsTest.kt (1)
39-45: Harden test isolation for the global steps provider.Nice cleanup in
tearDown(). To make this class fully order-independent, also resetPreInstallSteps.setStepsProviderForTests(null)insetUp()before stubbing the container.Proposed patch
`@Before` fun setUp() { + PreInstallSteps.setStepsProviderForTests(null) container = mockk(relaxed = true) gameDir = createTempDirectory(prefix = "preinstall-steps-test").toFile() every { container.drives } returns "A:${gameDir.absolutePath}" every { container.containerVariant } returns Container.BIONIC }Also applies to: 47-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/PreInstallStepsTest.kt` around lines 39 - 45, The test setUp currently stubs container state without resetting the global provider; call PreInstallSteps.setStepsProviderForTests(null) at the start of setUp() (before mocking/stubbing container and gameDir) to ensure the global steps provider is cleared and the test is order-independent; update the setUp() method in PreInstallStepsTest (and the other similar setup at lines 47-50) to invoke PreInstallSteps.setStepsProviderForTests(null) before creating mocks and setting every { container... }.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/test/java/app/gamenative/gamefixes/GOGDependencyFixTest.kt`:
- Around line 38-41: The teardown uses a global MockK cleanup (unmockkAll())
which can affect other tests; replace it with a scoped cleanup by calling
unmockkObject(GOGService.Companion) in the tearDown() of GOGDependencyFixTest to
match the mockkObject(GOGService.Companion) used in the test setup (refer to the
tearDown function and the mockkObject call for GOGService.Companion).
---
Nitpick comments:
In `@app/src/test/java/app/gamenative/utils/preInstallSteps/OpenALStepTest.kt`:
- Around line 22-26: Tests create a temporary directory in setUp (gameDir in
OpenALStepTest) but never remove it; add an `@After` annotated teardown method
(e.g., tearDown) that deletes the temp directory (call
gameDir.deleteRecursively() or equivalent) to clean up after each test and
prevent temp-dir buildup across runs.
In `@app/src/test/java/app/gamenative/utils/PreInstallStepsTest.kt`:
- Around line 39-45: The test setUp currently stubs container state without
resetting the global provider; call
PreInstallSteps.setStepsProviderForTests(null) at the start of setUp() (before
mocking/stubbing container and gameDir) to ensure the global steps provider is
cleared and the test is order-independent; update the setUp() method in
PreInstallStepsTest (and the other similar setup at lines 47-50) to invoke
PreInstallSteps.setStepsProviderForTests(null) before creating mocks and setting
every { container... }.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a088745e-b202-412d-b350-e79d79ef75e7
📒 Files selected for processing (17)
app/src/main/java/app/gamenative/gamefixes/GameFixesRegistry.ktapp/src/main/java/app/gamenative/utils/LaunchDependencies.ktapp/src/main/java/app/gamenative/utils/PreInstallSteps.ktapp/src/test/java/app/gamenative/gamefixes/GOGDependencyFixTest.ktapp/src/test/java/app/gamenative/gamefixes/GameFixesRegistryTest.ktapp/src/test/java/app/gamenative/gamefixes/KeyedGameFixTypesTest.ktapp/src/test/java/app/gamenative/gamefixes/LaunchArgFixTest.ktapp/src/test/java/app/gamenative/gamefixes/RegistryKeyFixTest.ktapp/src/test/java/app/gamenative/utils/LaunchDependenciesTest.ktapp/src/test/java/app/gamenative/utils/PreInstallStepsTest.ktapp/src/test/java/app/gamenative/utils/launchdependencies/GogScriptInterpreterDependencyTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/GogScriptInterpreterStepTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/OpenALStepTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/PhysXStepTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/UbisoftConnectStepTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.ktapp/src/test/java/app/gamenative/utils/preInstallSteps/XnaFrameworkStepTest.kt
✅ Files skipped from review due to trivial changes (6)
- app/src/test/java/app/gamenative/utils/preInstallSteps/XnaFrameworkStepTest.kt
- app/src/test/java/app/gamenative/utils/preInstallSteps/VcRedistStepTest.kt
- app/src/test/java/app/gamenative/utils/preInstallSteps/GogScriptInterpreterStepTest.kt
- app/src/test/java/app/gamenative/utils/LaunchDependenciesTest.kt
- app/src/test/java/app/gamenative/gamefixes/GameFixesRegistryTest.kt
- app/src/test/java/app/gamenative/gamefixes/RegistryKeyFixTest.kt
🚧 Files skipped from review as they are similar to previous changes (7)
- app/src/test/java/app/gamenative/gamefixes/KeyedGameFixTypesTest.kt
- app/src/test/java/app/gamenative/utils/preInstallSteps/UbisoftConnectStepTest.kt
- app/src/main/java/app/gamenative/utils/LaunchDependencies.kt
- app/src/main/java/app/gamenative/utils/PreInstallSteps.kt
- app/src/test/java/app/gamenative/gamefixes/LaunchArgFixTest.kt
- app/src/test/java/app/gamenative/utils/preInstallSteps/PhysXStepTest.kt
- app/src/test/java/app/gamenative/utils/launchdependencies/GogScriptInterpreterDependencyTest.kt
unbelievableflavour
changed the title
unbelievableflavour
changed the title
unbelievableflavour
changed the title
unbelievableflavour
force-pushed
the
more-unit-tests
branch
from
5a65f01 to
0b4b4b8
Compare
unbelievableflavour
force-pushed
the
more-unit-tests
branch
from
0b4b4b8 to
37b31cf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Since I introduced a couple new key components in gamenative I thought it be good to include tests for those. New types for these should require to have tests at well so that at least these new sections are always reliable.
Tests for:
I've made the launchdeps, game fix registry, and preinstallsteps tests itself test independent of their children so they dont have to be updated on every new child types. (hence the small mock functions added to those so their lists can be mutated)
Recording
https://discord.com/channels/1378308569287622737/1491337710244331720
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by CodeRabbit