You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Bazel runfiles doesn't do directories, just files; for Mac/Linux the directory symlinks work as files, in Windows it does not work, need to get a file and then walk up to the parent directory
Guard one test and add synchronization to another
🔧 Implementation Notes
We could use a relative path without Runfiles, but this seemed easier fix.
🔄 Types of changes
Bug fix (backwards compatible)
PR Type
Bug fix, Tests
Description
Fix .NET tests on Windows with Bazel by handling runfiles directories
Prevent NUnit test parallelism to avoid driver instance conflicts
Add synchronization to WebElementTest.ShouldSubmitElement test
Guard Firefox signed directory addon test on Windows platform
Generic: Robust Error Handling and Edge Case Management
Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation
Status: Null path fallback: GetPath may return null because Path.GetDirectoryName(path) is not null-guarded if Rlocation returns null/empty, leading to potential downstream failures instead of graceful degradation.
The suggestion highlights that adding --workers=1 to disable NUnit parallelism, while fixing a bug, will slow down the test suite. It recommends this be a temporary measure, with a future goal of re-enabling parallel execution.
Why: The suggestion correctly identifies a significant performance regression by disabling parallelism with --workers=1, and rightly frames it as a strategic trade-off that should be revisited.
Medium
Possible issue
Prevent returning null or empty paths
Improve path resolution by checking if the resolved path from Rlocation is null or empty and falling back to the legacy logic. This prevents returning null or empty paths.
try
{
// For directories, locate a file inside and get parent (runfiles manifest only lists files)
- string path = Bazel.Runfiles.Create().Rlocation($"_main/common/extensions/{name}/manifest.json");- return Path.GetDirectoryName(path);+ string manifestPath = Bazel.Runfiles.Create().Rlocation($"_main/common/extensions/{name}/manifest.json");+ if (string.IsNullOrEmpty(manifestPath))+ {+ // Fallback for non-Bazel environments or if runfiles resolution fails.+ string sCurrentDirectory = AppDomain.CurrentDomain.BaseDirectory;+ string sFile = Path.Combine(sCurrentDirectory, "../../../../common/extensions/" + name);+ return Path.GetFullPath(sFile);+ }+ return Path.GetDirectoryName(manifestPath);
}
catch (FileNotFoundException)
{
string sCurrentDirectory = AppDomain.CurrentDomain.BaseDirectory;
string sFile = Path.Combine(sCurrentDirectory, "../../../../common/extensions/" + name);
return Path.GetFullPath(sFile);
}
Apply / Chat
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies two potential edge cases (empty string from Rlocation and null from Path.GetDirectoryName) and provides a robust fix that improves the reliability of path resolution.
Medium
Assert the wait result explicitly
Replace Assert.That(..., Throws.Nothing) with Assert.IsTrue(...) to correctly validate the boolean result of the WaitFor condition, ensuring the test fails if the navigation does not complete successfully.
-Assert.That(- () =>- WaitFor(- () => driver.Url.StartsWith(resultPage),- "We are not redirected to the resultPage after submitting web element"),- Throws.Nothing);+Assert.IsTrue(+ WaitFor(+ () => driver.Url.StartsWith(resultPage),+ "We are not redirected to the resultPage after submitting web element"),+ "Submit did not navigate to the result page within the timeout");
Apply / Chat
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly points out that Assert.That(..., Throws.Nothing) does not check the return value of the WaitFor method. Using Assert.IsTrue makes the test assertion more explicit and correct.
Medium
Handle empty path resolution results
Add a check to handle cases where runfiles.Rlocation returns a null or empty string for manifestPath, falling back to Path.GetFullPath(path) to prevent returning an invalid path.
// For directories, locate a file inside and get parent (runfiles manifest only lists files)
string manifestPath = runfiles.Rlocation($"_main/{path}/manifest.json");
+if (string.IsNullOrEmpty(manifestPath))+{+ return Path.GetFullPath(path);+}
return Path.GetDirectoryName(manifestPath) ?? Path.GetFullPath(path);
Apply / Chat
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies a potential bug where an empty string from Rlocation would lead to an invalid path being returned, and the proposed fix handles this edge case gracefully.
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
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.
User description
Breaking this out from #16818
💥 What does this PR do?
🔧 Implementation Notes
🔄 Types of changes
PR Type
Bug fix, Tests
Description
Fix .NET tests on Windows with Bazel by handling runfiles directories
Prevent NUnit test parallelism to avoid driver instance conflicts
Add synchronization to WebElementTest.ShouldSubmitElement test
Guard Firefox signed directory addon test on Windows platform
Diagram Walkthrough
File Walkthrough
WebExtensionTest.cs
Handle Bazel runfiles directory resolution on Windowsdotnet/test/common/BiDi/WebExtension/WebExtensionTest.cs
LocateRelativePathto handle Bazel runfiles directoryresolution on Windows
directory when direct path resolution fails
block
WebElementTest.cs
Add synchronization to element submit testdotnet/test/common/WebElementTest.cs
ShouldSubmitElementtest usingWaitForhelperFirefoxDriverTest.cs
Guard Firefox addon test and fix path resolutiondotnet/test/firefox/FirefoxDriverTest.cs
IgnorePlatformattribute toShouldInstallAndUninstallSignedDirAddontestdirectory addons
GetPathmethod to use Bazel Runfiles for directoryresolution
dotnet_nunit_test_suite.bzl
Disable NUnit test parallelism for Bazeldotnet/private/dotnet_nunit_test_suite.bzl
_NUNIT_ARGSconstant with--workers=1flaginstance
_NUNIT_ARGSto all browser test configurationsBUILD.bazel
Add Runfiles NuGet dependencydotnet/test/firefox/BUILD.bazel
RunfilesNuGet package dependency to Firefox test suite