Conversation
…iceBuilder validation MobileBy.AndroidDataMatcher/AndroidViewMatcher/WindowsAutomation/TizenAutomation, GuardClauses, and AppiumServiceBuilder.UsingDriverExecutable/WithEnvironment had no test coverage. These are pure logic and run without any emulator/simulator.
There was a problem hiding this comment.
🟡 Changes recommended
Moderate findings remain regarding CI selection and incomplete MobileBy coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds pure-logic NUnit coverage for MobileBy factories, GuardClauses, and AppiumServiceBuilder validation.
Changes:
- Tests MobileBy factory output, dispatch, and fallback behavior.
- Tests GuardClauses valid and invalid inputs.
- Tests AppiumServiceBuilder validation and fluent chaining.
File summaries
| File | Summary |
|---|---|
test/integration/Common/MobileByFactoryTest.cs |
Covers MobileBy factories. Four moderate comments remain: missing CI selection (2 votes), incomplete Windows/Tizen dispatch coverage (3 votes), missing selector assertion (1 vote), and missing FindElements fallback coverage (1 vote). |
test/integration/Common/GuardClausesTest.cs |
Covers guard-clause success and exception paths. |
test/integration/Common/AppiumServiceBuilderValidationTest.cs |
Covers builder validation. One moderate comment remains because CI does not select these tests (1 vote). |
Review details
Suppressed comments (3)
test/integration/Common/AppiumServiceBuilderValidationTest.cs:23
- These tests are not selected by any current CI test command: the unit-test workflow filters to
AppiumLocalServerLaunchingTest|DirectConnectTest|AppiumClientConfigTest, while the functional jobs only selectAndroid,CustomCommand, orIOS. As a result, this new validation coverage runs only when someone invokes the Common filter locally and can regress without CI detecting it; include the Common tests in the unit-test filter (or add a dedicated job) before relying on this coverage.
public class AppiumServiceBuilderValidationTest
test/integration/Common/MobileByFactoryTest.cs:83
- This case verifies the selected finder method but not the selector argument. A regression that forwards the wrong selector would still pass, unlike the DataMatcher test above; also assert that
finder.LastSelectorequals{'name':'withText'}.
Assert.That(finder.LastCalledMethod, Is.EqualTo(nameof(IFindByAndroidViewMatcher<IWebElement>.FindElementsByAndroidViewMatcher)));
test/integration/Common/MobileByFactoryTest.cs:92
- The fallback assertion covers only
FindElement;MobileBy.FindElementshas a separate unsupported-context branch that is not exercised by this test. Add the correspondingAssert.Throws<InvalidCastException>forby.FindElements(unsupportedContext)so the fallback behavior cannot regress independently.
Assert.Throws<InvalidCastException>((Action)(() => by.FindElement(unsupportedContext)));
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| using OpenQA.Selenium.Appium; | ||
| using OpenQA.Selenium.Appium.Interfaces; | ||
|
|
||
| namespace Appium.Net.Integration.Tests.Common |
Comment on lines
+101
to
+105
| private class FakeFinder : ISearchContext, | ||
| IFindByAndroidDataMatcher<IWebElement>, | ||
| IFindByAndroidViewMatcher<IWebElement>, | ||
| IFindByWindowsUIAutomation<IWebElement>, | ||
| IFindByTizenUIAutomation<IWebElement> |
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
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.
PR title
test: add unit tests for MobileBy factories, GuardClauses, AppiumServiceBuilder validation
Related issue
Closes # n/a
List of changes
test/integration/Common/MobileByFactoryTest.cs: coversMobileBy.AndroidDataMatcher,AndroidViewMatcher,WindowsAutomation,TizenAutomationstatic factory methods (type/ToString()checks, dispatch to the matching finder interface via a fakeISearchContext, and theInvalidCastExceptionfallback path). These factory methods had zero test references anywhere in the suite — only the underlyingBy*classes were exercised directly (e.g. inElementTestEspresso.cs).test/integration/Common/GuardClausesTest.cs: coversRequireNotNull,RequireIsPositive,RequirePercentagehappy-path and exception-path (includingParamName).test/integration/Common/AppiumServiceBuilderValidationTest.cs: coversAppiumServiceBuilder.UsingDriverExecutableandWithEnvironmentargument validation (null args, missing file, empty dict keys/values, and the happy-path fluent chaining).Types of changes
Tests
How they run: These are pure logic tests with no Appium server/device dependency — they build and pass with a plain
dotnet teston bothnet48andnet8.0. Note: they live under theAppium.Net.Integration.Tests.Commonnamespace, which is not currently matched by either CI job's filter (FullyQualifiedName~Android|FullyQualifiedName~CustomCommandorFullyQualifiedName~IOS). No CI workflow changes are included in this PR — happy to add a filter/job update in a follow-up if desired.Documentation
Details
Verified locally:
dotnet test test/integration/Appium.Net.Integration.Tests.csproj -f net8.0 --filter "FullyQualifiedName~Appium.Net.Integration.Tests.Common"→ 25 passed, 0 failed.