iframe-proxy

IgorMilavec · 2021-12-15T22:31:30Z

I have created some unit tests for async API as a basis for discussion.
@drieseng can you please have a look if I'm kicking in the right direction?

Basically I plan to duplicate these tests:

SftpFileStreamTest_Ctor_* -> SftpFileStreamTest_OpenAsync_*
SftpFileStreamTest_Read_* -> SftpFileStreamTest_ReadAsync_*
SftpClientTest.* -> SftpClientTest.*Async
Have I left out any major surface area?

I have removed unsupported targets from the Tests project. I have tried to list all the relevant (and supported) targets for each VS version. I've left net35 in as it is the last one running on 2.0 runtime.

…targets

IgorMilavec · 2021-12-17T08:59:28Z

@drieseng, looks like I have opened a can of worms by running the tests on net5.0. :) Some tests were using runtime dependent code and had timing issues. I hope I have fixed those tests.
However, the tests are still very fragile. They pass when run locally, but when run by AppVeyor, they fail in sporadic and mysterious ways.

What I propose to do:

Remove all the assertions on Exception.Message, as these are runtime and (English) culture dependent.
ExceptionShouldNotHaveFired provides no feedback as to what exception occurred. We should replace all the occurrences with:

[TestMethod]
public void ExceptionShouldNotHaveFired()
{
    if (_exceptionRegister.Count > 0)
    {
        throw new Exception("ForwardedPort rased an exception: " + _exceptionRegister[0].Exception.Message, _exceptionRegister[0].Exception);
    }
}

I have already replaced ExceptionShouldNotHaveFired in ForwardedPortDynamicTest_Dispose_PortStarted_ChannelNotBound and now it fails with:

  Failed ExceptionShouldNotHaveFired [97 ms]
  Error Message:
   Test method Renci.SshNet.Tests.Classes.ForwardedPortDynamicTest_Dispose_PortStarted_ChannelNotBound.ExceptionShouldNotHaveFired threw exception: 
System.Exception: ForwardedPortDynamic rased an exception: An established connection was aborted by the software in your host machine. ---> System.Net.Sockets.SocketException: An established connection was aborted by the software in your host machine.
  Stack Trace:
      at System.Net.Sockets.Socket.Receive(Byte[] buffer, Int32 offset, Int32 size, SocketFlags socketFlags)
   at Renci.SshNet.Abstractions.SocketAbstraction.Read(Socket socket, Byte[] buffer, Int32 offset, Int32 size, TimeSpan readTimeout) in C:\projects\ssh-net\src\Renci.SshNet\Abstractions\SocketAbstraction.cs:line 371
   at Renci.SshNet.Abstractions.SocketAbstraction.ReadByte(Socket socket, TimeSpan timeout) in C:\projects\ssh-net\src\Renci.SshNet\Abstractions\SocketAbstraction.cs:line 289
   at Renci.SshNet.ForwardedPortDynamic.HandleSocks(IChannelDirectTcpip channel, Socket clientSocket, TimeSpan timeout) in C:\projects\ssh-net\src\Renci.SshNet\ForwardedPortDynamic.NET.cs:line 189
--- End of inner exception stack trace ---
    at Renci.SshNet.Tests.Classes.ForwardedPortDynamicTest_Dispose_PortStarted_ChannelNotBound.ExceptionShouldNotHaveFired() in C:\projects\ssh-net\src\Renci.SshNet.Tests\Classes\ForwardedPortDynamicTest_Dispose_PortStarted_ChannelNotBound.cs:line 135

"An established connection was aborted by the software in your host machine" seems to be the prevailing cause for failures on AppVeyor. I don't really know how to approach this. Could we at least mark the tests as inconclusive instead of failing them in case of this exception?

The test SendMessageOnSessionShouldBeInvokedThreeTimes consistently fails when run on net5.0:

  Failed SendMessageOnSessionShouldBeInvokedThreeTimes [376 ms]
  Error Message:
   Test method Renci.SshNet.Tests.Classes.BaseClientTest_Connected_KeepAliveInterval_NotNegativeOne.SendMessageOnSessionShouldBeInvokedThreeTimes threw exception: 
Moq.MockException: 
Expected invocation on the mock exactly 3 times, but was 4 times: p => p.TrySendMessage(It.IsAny<IgnoreMessage>())
Performed invocations:
   Mock<ISession:12> (p):
      ISession.add_HostKeyReceived(EventHandler<HostKeyEventArgs>)
      ISession.add_ErrorOccured(EventHandler<ExceptionEventArgs>)
      ISession.Connect()
      ISession.IsConnected
      ISession.TrySendMessage(SSH_MSG_IGNORE)
      ISession.TrySendMessage(SSH_MSG_IGNORE)
      ISession.TrySendMessage(SSH_MSG_IGNORE)
      ISession.TrySendMessage(SSH_MSG_IGNORE)
  Stack Trace:
      at Moq.Mock.Verify(Mock mock, LambdaExpression expression, Times times, String failMessage) in C:\projects\moq4\src\Moq\Mock.cs:line 337
   at Moq.Mock`1.Verify[TResult](Expression`1 expression, Times times) in C:\projects\moq4\src\Moq\Mock`1.cs:line 835
   at Renci.SshNet.Tests.Classes.BaseClientTest_Connected_KeepAliveInterval_NotNegativeOne.SendMessageOnSessionShouldBeInvokedThreeTimes() in C:\projects\ssh-net\src\Renci.SshNet.Tests\Classes\BaseClientTest_Connected_KeepAliveInterval_NotNegativeOne.cs:line 97

As you have more insight into the codebase I kindly ask you if you can have a look at it

zybexXL · 2021-12-17T11:01:37Z

I'm also seeing AppVeyor failing a lot due to port exhaustion - I mentioned this before on #877 (comment)

…sConnectedAndChannelIsOpen

zybexXL · 2021-12-17T16:34:32Z

Still failing on AppVeyor, seems to be a different issue:

Test method Renci.SshNet.Tests.Classes.BaseClientTest_Connected_KeepAliveInterval_NotNegativeOne.SendMessageOnSessionShouldBeInvokedThreeTimes threw exception: 
Moq.MockException: 
Expected invocation on the mock exactly 3 times, but was 4 times: p => p.TrySendMessage(It.IsAny<IgnoreMessage>())

IgorMilavec · 2021-12-17T17:02:32Z

Lots o' issues. :) That is the last non-sporadic one, hopefully it is fixed now...

…different targets

IgorMilavec · 2022-02-10T22:01:11Z

@drieseng, can you please have a look a this? In addition to adding coverage it also fixes a lot of transient errors, which blocks a lot of other PRs.

drieseng

Overall this PR looks great.
I indicated a few changes that I'd like to move out of this PR.
In general, please try to have smaller PRs.
PS. I happen to get carried away myself, so don't hesitate to remind me of this as well (and reject my PRs for the same reason).

drieseng · 2022-02-11T21:54:35Z

        public void SendMessageOnSessionShouldBeInvokedThreeTimes()
        {
-            _sessionMock.Verify(p => p.TrySendMessage(It.IsAny<IgnoreMessage>()), Times.Exactly(3));
+            Assert.AreEqual(3, _actualKeepAliveCount);


Why do you prefer this over verifying the mock?

It was the only way I found to stop this test failing sporadically for me. Let me remove non-Async changes from the PR and then we'll disuss, OK?

OK. I flagged some other changes that you can keep in this PR.

and .... THANKS!

drieseng · 2022-02-11T21:55:59Z

 os: Visual Studio 2019

+init:
+ - netsh int ipv4 set dynamicport tcp start=1025 num=64510


Did this resolve any issues?

It was more of a "we open and close a lot of ports in tests and the tests are sporadically failing with connection failures, so more client-side ports can't hurt" line of reasoning.

drieseng · 2022-02-11T21:57:16Z

-    vstest.console /logger:Appveyor src\Renci.SshNet.Tests\bin\Debug\net40\Renci.SshNet.Tests.dll /TestCaseFilter:"TestCategory!=integration&TestCategory!=LongRunning"
+    vstest.console /logger:Appveyor src\Renci.SshNet.Tests\bin\Debug\net472\Renci.SshNet.Tests.dll /TestCaseFilter:"TestCategory!=integration&TestCategory!=LongRunning"

-    vstest.console /logger:Appveyor src\Renci.SshNet.Tests\bin\Debug\net35\Renci.SshNet.Tests.dll /TestCaseFilter:"TestCategory!=integration&TestCategory!=LongRunning"


I'd first like to discuss dropping .NET 3.5 support.
Perhaps we'll have a final release supporting the legacy frameworks.
As I said, let's discuss this first.

drieseng · 2022-02-11T21:58:09Z


-            if (_channelThread != null)
-            {
-                if (_channelThread.IsAlive)


I suppose you did this because Thread.Abort is no longer supported on .NET (Core).

drieseng · 2022-02-11T21:59:06Z

                _remoteWindowSize,
                _remotePacketSize);

+            ManualResetEvent isReady = new ManualResetEvent(false);
