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

Fix #1238 - Only consider instance constructors #1239

Merged
merged 3 commits into from
Dec 12, 2020

Conversation

tillig
Copy link
Member

@tillig tillig commented Dec 11, 2020

#1238 shows that when we query types for constructors, usually when building reflection activators, we're inadvertently getting static constructors along with instance constructors. Since we can't create an instance of a type based on a static constructor, this PR removes the BindingFlags.Static when searching for constructors.

@@ -1,4 +1,5 @@
{
"dotnet-test-explorer.testProjectPath": "test/**/*Test.csproj",
Copy link
Member Author

Choose a reason for hiding this comment

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

This helps avoid showing the benchmark tests in the VS Code test explorer so you can just "run all tests" without having to worry about eating the next 45 minutes with benchmarks.

@@ -41,5 +42,176 @@ public void AnOpenGenericTypeIsNotAClosedTypeOfAnything()
{
Assert.False(typeof(CommandBase<>).IsClosedTypeOf(typeof(CommandBase<>)));
}

Copy link
Member Author

Choose a reason for hiding this comment

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

We didn't really have any test coverage specifically around these extensions so I added it to ensure the update to the constructor search flags didn't break anything else.

@codecov
Copy link

codecov bot commented Dec 11, 2020

Codecov Report

Merging #1239 (3ca19a7) into develop (8830021) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1239   +/-   ##
========================================
  Coverage    27.77%   27.77%           
========================================
  Files           11       11           
  Lines           18       18           
  Branches         1        1           
========================================
  Hits             5        5           
  Misses          12       12           
  Partials         1        1           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 180de40...3ca19a7. Read the comment docs.

Copy link
Member

@alistairjevans alistairjevans left a comment

Choose a reason for hiding this comment

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

Looks great! 🎢

@alistairjevans alistairjevans merged commit 88aa162 into develop Dec 12, 2020
@tillig tillig deleted the feature/issue-1238 branch January 22, 2021 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants