Fix PackageFeedSortingTest functional test #6185
Conversation
chenriksson
force-pushed
the
chenriks-testfix-feedsort
branch
from
cf7619b to
bf94171
Compare
chenriksson
requested review from
joelverhagen,
loic-sharma,
shishirx34,
ryuyu,
cristinamanum,
skofman1 and
scottbommarito
| @@ -196,47 +197,67 @@ private static string GetPackagesAppearInFeedInOrderUrl(DateTime time, string ti | |||
| var request = WebRequest.Create(UrlHelper.V2FeedRootUrl + @"stats/downloads/last6weeks/"); | |||
| var response = await request.GetResponseAsync(); | |||
|
|
|||
| string responseText; | |||
| using (var sr = new StreamReader(response.GetResponseStream())) | |||
| string responseText = await new StreamReader(response.GetResponseStream()) | |||
There was a problem hiding this comment.
Why were the usings block for the StreamReaders removed?
There was a problem hiding this comment.
Oversight in refactoring - fixed!
| } | ||
|
|
||
| // skip over `"PackageId": ` | ||
| feedResponseText = feedResponseText.Substring(start + 14); |
There was a problem hiding this comment.
What do you think of using @"""PackageId"": ".Length instead of 14?
chenriksson
force-pushed
the
chenriks-testfix-feedsort
branch
from
bf94171 to
936f829
Compare
| request = WebRequest.Create(UrlHelper.BaseUrl + @"stats/packageversions"); | ||
| private string[] GetPackageNamesFromFeedResponse(string feedResponseText) | ||
| { | ||
| const string PackageIdStartKey = @"""PackageId"": """; |
There was a problem hiding this comment.
IMO the \" syntax is a little easier to read, you've got like a billion "s in here
| break; | ||
| } | ||
|
|
||
| feedResponseText = feedResponseText.Substring(start + PackageIdStartKey.Length); |
There was a problem hiding this comment.
The response is simple JSON, I think we could easily parse it with JSON.NET and this code would be much simpler.
var packages = JArray.Parse(feedResponseText);
foreach (var package in packages)
{
yield return ((JObject)package)["PackageId"].Value<string>();
}
Could also model it and use JsonConvert.DeserializeObject, e.g.
private class Last6WeeksStatistics
{
public IEnumerable<PackageStatistic> PackageStatistics { get; set; }
}
private class PackageStatistic
{
public string PackageId { get; set; }
}
| responseText = await sr.ReadToEndAsync(); | ||
| // Check to make sure the top 10 packages are in the same order as the feed. | ||
| // We add angle brackets to prevent false failures due to duplicate package names in the page. | ||
| var condition = statsResponse.IndexOf(">" + last + "<", StringComparison.Ordinal) |
There was a problem hiding this comment.
This is pretty hacky--we're asserting the order of an HTML table for looking for the index of >{package ID}< and so on.
The real structure is:
<table>
<tbody>
<tr>
<td>{row number}<td>
<td><a href="/packages/{package ID}">{package ID}</a><td>
...
</tr>
</tbody>
</table>
I guess this is sufficient for now, but this code would be much more readable if you could find some sort of actual HTML parser for this purpose.
|
@scottbommarito My goal was just the bug fix, but did some light refactoring while there. Not sure it's worth revisiting the test for heavier refactoring. |
Fixes #6168 (functional test failure when <10 distinct packages in feed)