Skip to content

Fix unit test failures#343

Open
aled wants to merge 7 commits into
TalAloni:masterfrom
aled:master
Open

Fix unit test failures#343
aled wants to merge 7 commits into
TalAloni:masterfrom
aled:master

Conversation

@aled
Copy link
Copy Markdown

@aled aled commented Apr 2, 2026

Running the unit tests in my environment resulted in some errors, which are fixed by these updates.

@TalAloni
Copy link
Copy Markdown
Owner

TalAloni commented Apr 2, 2026

Thanks,
There are actually two unrelated fixes here,

  • Regarding port randomization, there is always a chance of this issue happening, how common it is for you? I'm someone reluctant to replace one random function with another given that this is not a common issue for me.
  • using the test directory is better than using the system test folder. I agree C:\Tests will not make sense for everyone.

@aled
Copy link
Copy Markdown
Author

aled commented Apr 2, 2026

Regarding the port randomization issue, the test fails almost every time for me (using the test runner built in to JetBrains Rider). One of the tests succeeds and the other gets a "port in use" error. I believe this is because it uses the time as the random seed, and the test runner runs tests in parallel, so they run at the exact same time.

Note my fix still gives a 1/60000ish chance of failure. Alternatively I could write a static method that randomizes the port once, then increments it for each subsequent call. Or catches the error when it happens and increments the port number. Whatever you like.

@TalAloni
Copy link
Copy Markdown
Owner

TalAloni commented Apr 3, 2026

Alternatively I could write a static method that randomizes the port once, then increments it for each subsequent call.

That's what I would have done, with Interlocked.Increment

@aled
Copy link
Copy Markdown
Author

aled commented Apr 4, 2026

Updated to use Interlocked.Increment.

public class NTDirectoryFileSystemTests : NTFileStoreTests
{
private static readonly string TestDirectoryPath = @"C:\Tests";
private static readonly string TestDirectoryPath = Path.Combine(Path.GetTempPath(), "SMBLibraryTests");
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid mixing two unrelated changes in the same pull request

{
var next = Interlocked.Increment(ref m_serverPort);

if (next > maxPort)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be simpler to avoid a possible random value near port 65535 then to have this check

{
m_serverPort = 1000 + new Random().Next(50000);
m_tcpListener = new TcpListener(IPAddress.Loopback, m_serverPort);
m_tcpListener = new TcpListener(IPAddress.Loopback, GetNextPortNumber());
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use
int port = Interlocked.Increment(ref s_serverPort);
No need for a method

// by incrementing the port number for each test
private static readonly int minPort = 1025;
private static readonly int maxPort = 65535;
private static int m_serverPort = minPort + new Random().Next(maxPort - minPort);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename to s_serverPort (I prefer s_ prefix for static variables)

@TalAloni TalAloni force-pushed the master branch 3 times, most recently from 57cfb6a to 336c47f Compare April 20, 2026 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants