-
Notifications
You must be signed in to change notification settings - Fork 458
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
Longhaul: Tolerance adjustments #5916
Merged
kodiakhq
merged 7 commits into
Azure:master
from
and-rewsmith:andsmi/master-longhaul-tol
Jan 20, 2022
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
6a5c736
squash dev commits
and-rewsmith 5651efc
self review
and-rewsmith 5283424
better documentation for test report tolerances
and-rewsmith 97a13fb
fix comment
and-rewsmith 0ca4979
Merge branch 'master' into andsmi/master-longhaul-tol
and-rewsmith da31e6d
Merge branch 'master' into andsmi/master-longhaul-tol
and-rewsmith a0db1e0
Merge branch 'master' into andsmi/master-longhaul-tol
and-rewsmith File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,8 @@ namespace TestResultCoordinator.Reports.LegacyTwin | |
|
||
sealed class LegacyTwinReportGenerator : ITestResultReportGenerator | ||
{ | ||
const double BigToleranceProportion = .005; | ||
const double LittleToleranceProportion = .001; | ||
static readonly ILogger Logger = ModuleUtil.CreateLogger(nameof(LegacyTwinReportGenerator)); | ||
readonly string trackingId; | ||
|
||
|
@@ -47,15 +49,9 @@ public async Task<ITestResultReport> CreateReportAsync() | |
{ | ||
Logger.LogInformation($"Start to generate report by {nameof(LegacyTwinReportGenerator)} for Sources [{this.SenderSource}] "); | ||
IDictionary<int, int> results = new Dictionary<int, int>(); | ||
bool isPassed = true; | ||
while (await this.SenderTestResults.MoveNextAsync()) | ||
{ | ||
int status = int.Parse(this.SenderTestResults.Current.Result.Substring(0, 3)); | ||
if (status > 299) | ||
{ | ||
isPassed = false; | ||
} | ||
|
||
if (results.ContainsKey(status)) | ||
{ | ||
results[status] = results[status] + 1; | ||
|
@@ -66,6 +62,8 @@ public async Task<ITestResultReport> CreateReportAsync() | |
} | ||
} | ||
|
||
bool isPassed = this.IsPassed(results); | ||
|
||
var report = new LegacyTwinReport( | ||
this.TestDescription, | ||
this.trackingId, | ||
|
@@ -77,5 +75,78 @@ public async Task<ITestResultReport> CreateReportAsync() | |
Logger.LogInformation($"Successfully finished creating LegacyTwinReport for Source [{this.SenderSource}]"); | ||
return report; | ||
} | ||
|
||
// Tolerances are needed due to a combination of false-positive failures and real product-issues. | ||
// - [Nested-Edge] [Broker-Enabled]: Sometimes we get an excessive amount of 501 and 504 status codes. | ||
// Fail the tests if we have > 0.1% of either code. | ||
// + (501) We don't receive some desired properties in module-registered twin desired property callback | ||
// + (504) Module cannot make reported property update | ||
bool IsPassed(IDictionary<int, int> statusCodesToCount) | ||
{ | ||
bool isPassed = true; | ||
int totalResults = statusCodesToCount.Sum(x => x.Value); | ||
|
||
if (totalResults == 0) | ||
{ | ||
return false; | ||
} | ||
|
||
if (this.Topology == Topology.Nested && this.MqttBrokerEnabled) | ||
{ | ||
// See TwinTester/StatusCode.cs for reference. | ||
int[] bigToleranceStatusCodes = { }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed everything requiring a big tolerance, as the case that made me initially create this hasn't reproduced. But chose to leave it for now in case we need it later. |
||
int[] littleToleranceStatusCodes = { 501, 504 }; | ||
isPassed = this.GeneratePassResult(statusCodesToCount, bigToleranceStatusCodes, littleToleranceStatusCodes); | ||
} | ||
else | ||
{ | ||
List<int> statusCodes = statusCodesToCount.Keys.ToList(); | ||
IEnumerable<int> failingStatusCodes = statusCodes.Where(s => | ||
{ | ||
string statusCode = s.ToString(); | ||
return !statusCode.StartsWith("2"); | ||
}); | ||
|
||
isPassed = failingStatusCodes.Count() == 0; | ||
} | ||
|
||
return isPassed; | ||
} | ||
|
||
bool GeneratePassResult(IDictionary<int, int> statusCodesToCount, int[] bigToleranceStatusCodes, int[] littleToleranceStatusCodes) | ||
{ | ||
int totalResults = statusCodesToCount.Sum(x => x.Value); | ||
foreach (KeyValuePair<int, int> statusCodeToCount in statusCodesToCount) | ||
{ | ||
int statusCode = statusCodeToCount.Key; | ||
int statusCodeCount = statusCodeToCount.Value; | ||
|
||
// ignore the status codes indicating some success | ||
if (statusCode.ToString().StartsWith("2")) | ||
{ | ||
continue; | ||
} | ||
else if (bigToleranceStatusCodes.Contains(statusCode)) | ||
{ | ||
if ((double)statusCodeCount / totalResults > BigToleranceProportion) | ||
{ | ||
return false; | ||
} | ||
} | ||
else if (littleToleranceStatusCodes.Contains(statusCode)) | ||
{ | ||
if ((double)statusCodeCount / totalResults > LittleToleranceProportion) | ||
{ | ||
return false; | ||
} | ||
} | ||
else | ||
{ | ||
return false; | ||
} | ||
} | ||
|
||
return true; | ||
} | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Does this parse command always work? Maybe you can use TryParse here to guarantee that you don't get any exceptions.
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 haven't seen this ever fail. Since I didn't change this in the PR and it isn't a problem, I'd rather leave it as-is.