Skip to content
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

Being able to navigate to a PR page of an AppVeyor build #6326

Merged

Conversation

pmiossec
Copy link
Member

@pmiossec pmiossec commented Mar 1, 2019

Proposed changes

AppVeyor build report contains the data to get the url of the Pull request page.
So add an menu item in the contextual revision grid menu to be able to open the page.

Obviously, this menu item is only displayed when a PR has been built.

Support of GitHub, bitbucket and gitlab for the moment.

PR icon is from https://octicons.github.com/ (MIT licence)

Screenshots

image

Test methodology

  • Manual

Test environment(s)

  • Git Extensions 3.1.0
  • Build 41f46f7 (Dirty)
  • Git 2.20.0.windows.1
  • Microsoft Windows NT 10.0.17134.0
  • .NET Framework 4.7.3324.0
  • DPI 192dpi (200% scaling)

@codecov
Copy link

codecov bot commented Mar 1, 2019

Codecov Report

Merging #6326 into master will increase coverage by 0.35%.
The diff coverage is 54.91%.

@@            Coverage Diff             @@
##           master    #6326      +/-   ##
==========================================
+ Coverage   46.28%   46.64%   +0.35%     
==========================================
  Files         683      687       +4     
  Lines       51297    51641     +344     
  Branches     6729     6805      +76     
==========================================
+ Hits        23741    24086     +345     
+ Misses      26330    26248      -82     
- Partials     1226     1307      +81
Flag Coverage Δ
#production 36.09% <37.2%> (+0.46%) ⬆️
#tests 97.53% <97.22%> (-0.01%) ⬇️

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

I'd like to see tests for the new functionality.
It would be good to have tests QueryBuildsResults as well. To achieve that it is better to have it refactored by extracting everything below var result = await GetResponseAsync(... line into a separate method.

@@ -189,6 +189,24 @@ private class Project
public string QueryUrl;
}

private string BuildPullRequetUrl(string repositoryType, string repositoryName, string pullRequestId)
{
switch (repositoryType)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
switch (repositoryType)
switch (repositoryType.ToLowerInvariant())

(though ToUpperInvariant is faster)

return null;
}

return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant

@pmiossec pmiossec force-pushed the open_pr_page_from_appveyor_build branch from 90ae937 to 5cf47f0 Compare March 4, 2019 08:59
@pmiossec
Copy link
Member Author

pmiossec commented Mar 4, 2019

done.

@pmiossec pmiossec force-pushed the open_pr_page_from_appveyor_build branch 3 times, most recently from f18dd26 to ffb0014 Compare March 5, 2019 08:47
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, few minor tweaks

buildInfo.Should().HaveCount(0);
}

private const string ContentBegin = @"{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move large chunks of input data into physical files and assert the result with ApprovalTests.
Load:

public void Deserialize_should_load_legacy_categorised_repositories()
{
var xml = EmbeddedResourceLoader.Load(Assembly.GetExecutingAssembly(), $"{GetType().Namespace}.MockData.CategorisedRepositories01.xml");
if (string.IsNullOrWhiteSpace(xml))
{
throw new FileFormatException("Unexpected data");
}

Assert (few overloads available):


This will create a testname.approved.txt file that can be viewed in a normal diff viewer. The .approved. files gets committed into the repo.

This will significantly simplify the maintenance of tests and change of data, if necessary.

GitUI/Properties/Images.resx Show resolved Hide resolved
@pmiossec pmiossec force-pushed the open_pr_page_from_appveyor_build branch from ffb0014 to ae455c0 Compare March 6, 2019 13:24
@pmiossec pmiossec closed this Mar 6, 2019
@pmiossec pmiossec reopened this Mar 6, 2019
@pmiossec pmiossec force-pushed the open_pr_page_from_appveyor_build branch from 0f29c99 to ccb4df0 Compare March 6, 2019 14:06
@pmiossec
Copy link
Member Author

pmiossec commented Mar 6, 2019

done.

Let's move large chunks of input data into physical files and assert the result with ApprovalTests.

I choose to serialize in Yaml because it's easy to specify a customer serializer (maybe it's possible in json or xml without having to change the tested code....). Indeed, the xml serializer throw due to unable to serialize an interface and json serializer just ignored the data so the test would not have verified all the datas we wanted.

I also have updated the .gitattributes to be able to see the diff on 'approval' files.
And I have added the AppVeyorReporter (because otherwise you can't see why a test crash) but could not have tested it because I solved my failing test on the CI build without it ;p

.gitattributes Outdated
@@ -7,4 +7,4 @@
*.sh text eol=lf
*.py text eol=lf
*.bin binary
*.approved.* binary
*.approved.* -text
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this line completely
https://github.com/approvals/ApprovalTests.Net#approved-file-artifacts

Some years ago it was necessary because EOL would change and fail tests.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, haven't tested

@pmiossec
Copy link
Member Author

I think we can remove this line completely

done.

@pmiossec pmiossec force-pushed the open_pr_page_from_appveyor_build branch from 4e04408 to 164b1d3 Compare March 20, 2019 08:54
@RussKie RussKie merged commit 64e0db6 into gitextensions:master Mar 20, 2019
@RussKie
Copy link
Member

RussKie commented Mar 20, 2019

Thank you

@RussKie RussKie added this to the 3.1.0 milestone Mar 20, 2019
@pmiossec pmiossec deleted the open_pr_page_from_appveyor_build branch March 20, 2019 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants