Skip to content

Conversation

@scamille
Copy link
Member

@scamille scamille commented Jun 7, 2021

If canceled during data transmission, #349 adds code to ensure that the connection automatically gets canceled.

Add a test to ensure this works.
Also, add an inverted version to ensure that cancellation before sending data does not result in cancellation.

If cancelled during data transmission, S7NetPlus#349 adds code to ensure that the connection automatically gets cancelled.

Add inerted test to ensure that cancellation before sending data does not result in a cancellation.
@scamille
Copy link
Member Author

scamille commented Jun 7, 2021

Works on my machine :-/

@mycroes
Copy link
Member

mycroes commented Jun 7, 2021

I get the same error locally, pay attention that it only happens on net452, not the other targets.

@mycroes
Copy link
Member

mycroes commented Jun 7, 2021

Seems to me the issue is with cancellationSource.CancelAfter(TimeSpan.FromMilliseconds(5)). Perhaps sometimes that triggers before the actual method on net472. Not sure why that's included in both tests (I think it shouldn't be in either...).

@mycroes
Copy link
Member

mycroes commented Jun 7, 2021

The fact that the tests now fail on a single platform is really good though. I still only have AppVeyor as required check, but I'm considering to make them all required.

@scamille
Copy link
Member Author

scamille commented Jun 8, 2021

Right I thought I removed that line, that of course explains it, thanks.

It might sense to wait a bit more to see how reliable the new CI actions are. On the other I do know from the simulation craft/simc project that they are pretty reliable, since we have a lot more commits and longer tests over there, but not any .NET.
Macos jobs can sometimes fail but it is still rare.

@mycroes
Copy link
Member

mycroes commented Jun 10, 2021

Thanks a lot @scamille!

@mycroes mycroes merged commit b475aee into S7NetPlus:develop Jun 10, 2021
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