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

TestResult.cs casts ITestResult to TestResult #3423

Closed
tommikiviniemi-srs opened this issue Nov 12, 2019 · 2 comments · Fixed by #3629
Closed

TestResult.cs casts ITestResult to TestResult #3423

tommikiviniemi-srs opened this issue Nov 12, 2019 · 2 comments · Fixed by #3629
Milestone

Comments

@tommikiviniemi-srs
Copy link

tommikiviniemi-srs commented Nov 12, 2019

Hi.

The iteration of the "IEnumerable< ITestResult > Children" member in TestResult.cs on line 407 is unnecessarily casting the ITestResult to TestResult, making it hard/impossible to keep extended children in the list.

Could the enumeration be fixed to iterate on the ITestResult interface, without the unnecessary casting? There are other places in the code base where similar unnecessary casts take place, but this one place is a real pain for me.

Thanks.

@CharliePoole
Copy link
Contributor

IIRC, we did a pass to fix this at one time, but probably only caught explicit casts, not the implied cast you point out. At some point, I believe we decided to require all results to derive from the TestResult abstract class, but I don't think we have been consistent about it. IMO we should either lose the interface and require the class or remove all these casts, which are confusing at best.

As for this change, we would probably want to search for uses of non-interface methods of TestResult and ensure we are not breaking anything.

@tommikiviniemi-srs
Copy link
Author

Thanks, that would be great. I derived my own classes from the interface directly, which is why I ran into this problem. The abstract class on the other hand has several methods marked as non-virtual so I can't extend from it properly either.

Thanks for consideration,
-Tommi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants