-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use REST interface for AppVeyor #12
Conversation
@ngbrown : Thank you for your contribution 👍. I will take a look on this weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Small changes required.
@@ -0,0 +1,15 @@ | |||
namespace Microsoft.VisualStudio.TestPlatform.Extensions.Appveyor.TestLogger | |||
{ | |||
internal class AppveyorApiTests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed since I'm building JSON manually instead of using a library. - Removed.
/// <param name="events">Events that can be registered for.</param> | ||
/// <param name="testRunDirectory">Test Run Directory</param> | ||
public void Initialize(TestLoggerEvents events, string testRunDirectory) | ||
private AppveyorLoggerQueue queue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had accidentally added a mess of tab characters and spaces. Added .editorconfig
to enforce standardization.
/// </summary> | ||
/// <param name="events">Events that can be registered for.</param> | ||
/// <param name="testRunDirectory">Test Run Directory</param> | ||
public void Initialize(TestLoggerEvents events, string testRunDirectory) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
string filename = string.IsNullOrEmpty(e.Result.TestCase.Source) ? string.Empty : Path.GetFileName(e.Result.TestCase.Source); | ||
string outcome = e.Result.Outcome.ToString(); | ||
|
||
allArgs.Add("-Name " + name); | ||
allArgs.Add("-Framework " + e.Result.TestCase.ExecutorUri.ToString()); | ||
var testResult = new Dictionary<string, string>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix the indentation of this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
queue.Cancel(); | ||
try | ||
{ | ||
consumeTask.Wait(TimeSpan.FromSeconds(10)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just wondering if there is any particular reason behind choosing 10 sec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not specifically. The API calls are to a localhost
and I expected them to be fast. Just thought there should be something. Do you have a suggestion for a better length?
1bcfa35
to
349fdbe
Compare
Add .editorconfig to standardize tabs/spaces. Add ReSharper DotSettings. Add .gitattributes
@Faizan2304 I found an issue with how I was handling the Aync at the I also fixed up the build script quite a bit to perform two-phase compilation so that the NuGet packages just built will be the ones tested. The I've rebased everything for a clean set of commits. |
Use release channels of dotnet SDK and runtimes.
Use FullyQualifiedName for test names.
The use of a blocking command-line tool per test is quite slow. It caused my tests to be canceled at the 60 minute limit with only 2,559 completed.
The changes use a Async queue and HTTP batch POST to minimize the time impact on builds with a large number of tests. With these changes, all 5,781 test complete in 19 minutes.