-
Notifications
You must be signed in to change notification settings - Fork 1k
Revert PR_9884 “Migrate HtmlHelp to CsWin32” and add unit test #10435
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
Conversation
437618e
to
62acdb8
Compare
To confirm, was this issue with the metadata? |
public class HelpTests | ||
{ | ||
[Fact] | ||
public void ShowHelpTest() |
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.
@Epica3055 - this is the minimal test. Could you please add more coverage?
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.
@Epica3055 - this is the minimal test. Could you please add more coverage?
@Tanya-Solyanik Yeah sure.
I think there are three methods should be tested.
But not all of them involve using native dll. This one I wrote here definitely involves using that ( I ran this test before and after the PR ). So I guess maybe this one is enough?
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.
for example when I write one like this
[Fact]
public void ShowHelpTest()
{
Help.ShowHelp(null, "https://www.microsoft.com/");
}
it doesn't involve using native dll.
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 see, you are right, for this issue, one is enough. However we need code coverage for all methods on this control eventually. I thought that once you are working in this space, it's easy to add more tests.
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.
This Help
is like a general method, not for a specific Control.
8.0-testresult.mp4
I guess this issue can track all Control tests.
Yes, metadata is already fixed, thanks to Lachlan, do we know how much time it would take to update metadata and Win32? |
Can happen in a day or so depending on the teams. |
62acdb8
to
704722f
Compare
@Tanya-Solyanik yeah, no problem. |
Fixes #10410
Related to PR_9884
The issue resulted from PR_9884
Proposed changes