Assignment 9+10 #95
Assignment 9+10 #95
Conversation
Updated to fix long testing times on non Windows systems
chanderlud
left a comment
chanderlud
left a comment
There was a problem hiding this comment.
- Implement
PingProcess'public Task<PingResult> RunTaskAsync(string hostNameOrAddress)✔- First implement
public void RunTaskAsync_Success()test method to testPingProcess.RunTaskAsync()using"localhost". ✔ - Do NOT use async/await in this implementation. ✔
- First implement
- Implement
PingProcess'async public Task<PingResult> RunAsync(string hostNameOrAddress)✔- First implement the
public void RunAsync_UsingTaskReturn_Success()test method to testPingProcess.RunAsync()using"localhost"without using async/await. ✔ - Also implement the
async public Task RunAsync_UsingTpl_Success()test method to testPingProcess.RunAsync()using"localhost"but this time DO using async/await. ✔
- First implement the
- Add support for an optional cancellation token to the
PingProcess.RunAsync()signature. ✔
Inside thePingProcess.RunAsync()invoke the token'sThrowIfCancellationRequested()method so an exception is thrown. ✔
Test that, when cancelled from the test method, the exception thrown is anAggregateException✔ with aTaskCanceledExceptioninner exception ✔ using the test methodsRunAsync_UsingTplWithCancellation_CatchAggregateExceptionWrapping✔andRunAsync_UsingTplWithCancellation_CatchAggregateExceptionWrappingTaskCanceledException✔ respectively. - Complete/fix AND test
async public Task<PingResult> RunAsync(IEnumerable<string> hostNameOrAddresses, CancellationToken cancellationToken = default)which executes ping for an array of hostNameOrAddresses (which can all be "localhost") in parallel, adding synchronization if needed. ✔
NOTE:- The order of the items in the stdOutput is irrelevant and expected to be intermingled.
- StdOutput must have all the ping output returned (no lines can be missing) even though intermingled. ✔
- Implement AND test
public Task<int> RunLongRunningAsync(ProcessStartInfo startInfo, Action<string?>? progressOutput, Action<string?>? progressError, CancellationToken token)usingTask.Factory.StartNew()and invokingRunProcessInternalwith aTaskCreationvalue ofTaskCreationOptions.LongRunningand aTaskSchedulervalue ofTaskScheduler.Current. Returning aTask<PingResult>is also okay.
NOTE: This method does NOT useTask.Run.
Extra Credit
- Test and implement
PingProcess.RunAsync(System.IProgress<T> progess)so that you can capture the output as it occurs rather than capturing the output only when the process finishes. ✔
Fundamentals
- Place all shared project properties into a
Directory.Build.Propsfile. ✔ - Place all shared project items into a
Directory.Build.targetsfile. - Ensure nullable reference types is enabled ✔
- Ensure that you turn on code analysis for all projects(EnableNETAnalyzers) ✔
- Set
LangVersionand theTargetFrameworkto the latest released versions available (preview versions optional) ✔ - and enabled .NET analyzers for both projects ✔
- For this assignment, consider using
Assert.AreEqual<T>()(the generic version) ✔ - All of the above should be unit tested ✔
- Choose simplicity over complexity ✔
Looks like all the assignment requirements are taken care of correctly, I left a few comments on minor things I thought could be improved.
thigiang16
left a comment
thigiang16
left a comment
There was a problem hiding this comment.
Instructions
1.Implement PingProcess' public Task RunTaskAsync(string hostNameOrAddress) ✔
First implement public void RunTaskAsync_Success() test method to test PingProcess.RunTaskAsync() using "localhost". ✔
Do NOT use async/await in this implementation. ✔
2. Implement PingProcess' async public Task RunAsync(string hostNameOrAddress) ✔
First implement the public void RunAsync_UsingTaskReturn_Success() test method to test PingProcess.RunAsync() using "localhost" without using async/await. ✔
Also implement the async public Task RunAsync_UsingTpl_Success() test method to test PingProcess.RunAsync() using "localhost" but this time DO using async/await.✔
3. Add support for an optional cancellation token to the PingProcess.RunAsync() signature. ✔ Inside the PingProcess.RunAsync() invoke the token's ThrowIfCancellationRequested() method so an exception is thrown.✔
Test that, when cancelled from the test method, the exception thrown is an AggregateException ✔ with a TaskCanceledException inner exception ✔ using the test methods RunAsync_UsingTplWithCancellation_CatchAggregateExceptionWrapping ✔and RunAsync_UsingTplWithCancellation_CatchAggregateExceptionWrappingTaskCanceledException ✔ respectively.
4. Complete/fix AND test async public Task RunAsync(IEnumerable hostNameOrAddresses, CancellationToken cancellationToken = default) which executes ping for an array of hostNameOrAddresses (which can all be "localhost") in parallel, adding synchronization if needed. ✔ NOTE:
The order of the items in the stdOutput is irrelevant and expected to be intermingled. StdOutput must have all the ping output returned (no lines can be missing) even though intermingled. ✔
5. Implement AND test public Task RunLongRunningAsync(ProcessStartInfo startInfo, Action<string?>? progressOutput, Action<string?>? progressError, CancellationToken token) using Task.Factory.StartNew() and invoking RunProcessInternal with a TaskCreation value of TaskCreationOptions.LongRunning and a TaskScheduler value of TaskScheduler.Current. Returning a Task is also okay. NOTE: This method does NOT use Task.Run. ✔
Extra Credit
Test and implement PingProcess.RunAsync(System.IProgress progess) so that you can capture the output as it occurs rather than capturing the output only when the process finishes. ✔ Fundamentals
Place all shared project properties into a Directory.Build.Props file.
Place all shared project items into a Directory.Build.targets file. Ensure nullable reference types is enabled ✔
Ensure that you turn on code analysis for all projects(EnableNETAnalyzers) ✔
Set LangVersion and the TargetFramework to the latest released versions available (preview versions optional) ✔ and enabled .NET analyzers for both projects ✔
For this assignment, consider using Assert.AreEqual() (the generic version) ✔
All of the above should be unit tested ✔
Choose simplicity over complexity ❌
The implementation looks good. A few areas could be simplified to favor simplicity over complexity, like avoiding unnecessary Task.Run or reducing duplicate code. Consider reusing existing methods to make it cleaner. You might also add more tests to improve coverage.
|
Instructions The implementation looks good but try to save task.runs bc they eat memory |
This reverts commit 002f6dc.
SummarySummary
CoverageAssignment - 66.6%
|
Thanks for the suggestion! I did experiment with removing some of the Task.Run usages, but the refactors ended up breaking the CI tests as they changed the expected execution behavior. This would be a great candidate for optimization and refactoring in the future though. |
|
Implement PingProcess' public Task RunTaskAsync(string hostNameOrAddress) ✔ Extra Credit Fundamentals Good work. There's not much to mention than what's already been brought up, so I'll leave it at that! |
Joshua-Lester3
left a comment
Joshua-Lester3
left a comment
There was a problem hiding this comment.
Instructions
- Implement
PingProcess'public Task<PingResult> RunTaskAsync(string hostNameOrAddress)✔- First implement
public void RunTaskAsync_Success()test method to testPingProcess.RunTaskAsync()using"localhost". ✔ - Do NOT use async/await in this implementation. ✔
- First implement
- Implement
PingProcess'async public Task<PingResult> RunAsync(string hostNameOrAddress)✔- First implement the
public void RunAsync_UsingTaskReturn_Success()test method to testPingProcess.RunAsync()using"localhost"without using async/await. ✔ - Also implement the
async public Task RunAsync_UsingTpl_Success()test method to testPingProcess.RunAsync()using"localhost"but this time DO using async/await. ✔
- First implement the
- Add support for an optional cancellation token to the
PingProcess.RunAsync()signature. ✔
Inside thePingProcess.RunAsync()invoke the token'sThrowIfCancellationRequested()method so an exception is thrown. ✔
Test that, when cancelled from the test method, the exception thrown is anAggregateException✔ with aTaskCanceledExceptioninner exception ✔ using the test methodsRunAsync_UsingTplWithCancellation_CatchAggregateExceptionWrapping✔andRunAsync_UsingTplWithCancellation_CatchAggregateExceptionWrappingTaskCanceledException✔ respectively. - Complete/fix AND test
async public Task<PingResult> RunAsync(IEnumerable<string> hostNameOrAddresses, CancellationToken cancellationToken = default)which executes ping for an array of hostNameOrAddresses (which can all be "localhost") in parallel, adding synchronization if needed. ✔
NOTE:- The order of the items in the stdOutput is irrelevant and expected to be intermingled.
- StdOutput must have all the ping output returned (no lines can be missing) even though intermingled. ✔
- Implement AND test
public Task<int> RunLongRunningAsync(ProcessStartInfo startInfo, Action<string?>? progressOutput, Action<string?>? progressError, CancellationToken token)usingTask.Factory.StartNew()and invokingRunProcessInternalwith aTaskCreationvalue ofTaskCreationOptions.LongRunningand aTaskSchedulervalue ofTaskScheduler.Current. Returning aTask<PingResult>is also okay.
NOTE: This method does NOT useTask.Run.
Extra Credit
- Test and implement
PingProcess.RunAsync(System.IProgress<T> progess)so that you can capture the output as it occurs rather than capturing the output only when the process finishes. ✔
Fundamentals
- Place all shared project properties into a
Directory.Build.Propsfile. - Place all shared project items into a
Directory.Build.targetsfile. - Ensure nullable reference types is enabled ✔
- Ensure that you turn on code analysis for all projects(EnableNETAnalyzers) ✔
- Set
LangVersionand theTargetFrameworkto the latest released versions available (preview versions optional) ✔ - and enabled .NET analyzers for both projects ✔
- For this assignment, consider using
Assert.AreEqual<T>()(the generic version) ✔ - All of the above should be unit tested ✔
- Choose simplicity over complexity ✔
Great job!
| /// </summary> | ||
| public static bool IsLike(this string text, string pattern, char escapeCharacter) => | ||
| new WildcardPattern(pattern, escapeCharacter).IsMatch(text); | ||
| #pragma warning disable |
There was a problem hiding this comment.
Maybe Ben said to do this, so let me know if I missed that. If not, this either needs explanation, or to be removed.
| @@ -0,0 +1,1240 @@ | |||
| #pragma warning disable | |||
|
|
||
| namespace Assignment; | ||
|
|
||
| public class ClonePing : PingProcess |
There was a problem hiding this comment.
I might prefer either keeping this in the Test project, or using a mocking library in your tests.
| void updateStdOutput(string? line) => | ||
| (stringBuilder??=new StringBuilder()).AppendLine(line); | ||
|
|
||
| int exit = RunProcessInternal(StartInfo, updateStdOutput, updateStdOutput, default); |
There was a problem hiding this comment.
Is there a reason default was switched to updateStdOutput for the third param?
| try | ||
| { | ||
| Task<PingResult> task = Sut.RunAsync("localhost", token.Token); | ||
| task.Wait(); | ||
|
|
||
| } | ||
| catch (AggregateException ex) | ||
| { | ||
|
|
||
| Assert.IsInstanceOfType<AggregateException>(ex); | ||
| return; | ||
|
|
||
| } | ||
|
|
||
| Assert.Fail("Expected Aggregate Exception, but none was thrown"); |
There was a problem hiding this comment.
I'd probably do this using Assert.Throws: https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.testtools.unittesting.assert.throwsexception?view=mstest-net-3.11
7 participants
I worked with @Al3xramirez. We implemented the extra credit.