Set inner html as default to implict set ReturnType

[rwecho]

#507

[rwecho]

@elgonzo @JonathanMagnan Please assist in reviewing this pull request , thanks.

[JonathanMagnan]

Hello @rwecho ,

I didn't look yet at the test, but as for the code, from what I understand, it looks perfect to me.

I will try to take some time this week to fully read discussion #507 to make a double-check, but unless @elgonzo has something to say, we will try to deploy it at the beginning of next week.

Best Regards,

Jon

[elgonzo]

Aside from the missing linebreak between the get and set accessor of the XPathAttribute.NodeReturnType property, the HtmlNode.Encapsulator.cs changes look fine.

With respect to the tests, there is room for improvement in my opinion.

The test method(s) could do with a more meaningful and descriptive name, "Encapsulation_Test2" isn't giving much clue what aspect of the GetEncapsulatedData API is being tested.

That said, the current Encapsulation_Test2 method is attempting testing too much in a single test method. It is attempting too much, in the sense that creating and populating (nested) [HasXPath] properties (like those of the Link and FormName type) is a complex endeavour on its own, requiring multiple assertions in a test to confirm success that it in my opinion warrants own tests.

It is doing too little, as many assertions do not really positively affirm whether the correct result/behavior has been achieved by the GetEncapsulatedData API. (Like for example Assert.NotEmpty(testHtml.Link1) -- yeah, testHtml.Link1 not being null, but does it actually contain the correct expected content? Or, similarily, Assert.NotNull(testHtml.Link3) -- well, it got some Link instance, but has that instance actually being correctly populated? Etc...)

And, considering the update to the NodeReturnType handling this PR deals with, testing proper handling of the different NodeReturnType values are largely absent. Tests regarding different NodeReturnType values both in the context of "primitive" property types (like string or int, for example) and properties of [HasXPath] types.

I think i might try github Codespaces for the first time to whip something up regarding the tests. I never used github Codespaces before, so this might be a good opportunity for me to play around with it a little. But i can't for that reason not yet make promises as of how quick i will be or whether github Codespaces will turn out to be a dead end with regard to creating some PR and trying to run tests in the Codespaces environment...

(UPDATE re Codespaces: In faux French accent: "Five minutes later..." I started Codespaces and was instantly greeted by "An unexpected error occurred that requires a reload of this page. The workbench failed to connect to the server (Error: Connection error: Unauthorized client refused)". Well, that idea went down like a lead balloon...)

[rwecho]

The test Encapsulation_Test2 method, I really hope to test all the basic functionalities of Encapsulation API. But this is like integration testing rather than unit testing.

When I was writing this test, I felt that it became too verbose if I included everything. If I want to test it thoroughly, it might involve functionalities with other APIs, and it may require establishing a complete test framework to accomplish that.

For unit tests, what about your opinions? Maybe we can optimize it in further work.

I didn't use Codespaces either. I always thought it was something similar to Code-Server. Does it provide any benefits for unit testing?

[elgonzo]

The test Encapsulation_Test2 method, I really hope to test all the basic functionalities of Encapsulation API.

It sounds to me like you're on a road trying to overload a single test method with what should be individual test methods. A test method failing should tell the developer what's wrong with the tested code. A test method testing too many things can't do that easily, because such a test method failing would require the developer to first figure out which part of the test method failed. Of course you could instrument the test method with test output to help with this, but avoiding overloaded test methods naturally avoids this problem. (It goes without saying, when a test method is considered overloaded is subjective and less of a hard science, and there are no rules without exceptions...) Also, any assertion made in test methods need to positively affirm the intended behavior, otherwise the assertion has no real value. For example, an assertion about a result containing some string instance is typically not positively affirming the intended API behavior without asserting whether that string instance has the intended content.

But this is like integration testing rather than unit testing.

It's not integration testing that is needed. We are only dealing with testing a single API (GetEncapsulatedData), whose input are the HTML document and the attribute-annotated type(s) the HTML document should be transformed into. There is no other component, module or system to be concerned about here, hence why integration testing is not a concern in this respect.

When I was writing this test, I felt that it became too verbose if I included everything.

The baseline for tests included in a PR is that those tests should be covering the code changes in the PR and proving that the code changes do indeed have the intended effect. As far as this PR is concerned, don't worry about everything else other than NodeReturnValue-related stuff. As i already suggested, i am going to whip up tests covering the NodeReturnType-related functionality, with or without github codespace. So, don't worry if you don't feel like doing those NodeReturnValue-related tests, you have already done more than could be rightfully asked from you, and it is most appreciated.

I didn't use Codespaces either. [...] Does it provide any benefits for unit testing?

I can't tell, because i still didn't manage to get it working in my browser, and i don't feel like spending a lot of time playing around with browser installations, proxies/VPNs or possibly even VMs just trying to get a sodding web app to run... ;-P (no, using a fresh clean unspoiled browser profile didn't help, that was one of the things i tried in that five minutes i mentioned...) But no losses here, it was just curiosity that led me to wanting to try out Codespace. Well, and while my curiosity (fortunately) didn't kill a cat, Codespace refusing to run with a (to me meaningless) error message killed my curiosity :-P.

[rwecho]

I greatly appreciate your guidance in my work. Regarding testing, I also hope to collaborate with you to help do more. If needed, you can assign me some tasks.

[elgonzo]

@rwecho

I made a pull request for your fork of HAP (rwecho#1) with the unit tests relating to the GetEncapsulatedData behavior controlled by XPathAttribute.NodeReturnType. I did it that way so you can merge it with your fork and then subsequently integreate it with your pull request here.

Also take a look into the HtmlNode.Encapsulator.cs in my pull request. I made a few minor changes. The most significant one is removing the XPathAttribute constructor you added, as there is no point in specifying a ReturnType when the [XPath] annotation denotes an element attribute. Related to this, i added two more checks in the GetEncapsulatedData method throwing an exception if a user dares to explicitly set the NodeReturnType on such [XPath] annotations. And since i did introduce a specific exception type for this, i also went ahead and changed the Tools.GetHtmlForEncapsulation method to also throw this exception type instead of System.Exception.

Take a look at the changes in my PR first before committing it to this PR here. Two pairs of eyes are better than one, and maybe i did some dumb thing i didn't notice that you perhaps can spot...

Also, unfortunately the HtmlAgilityPack.NETStandard2_0 test project is using XUnit. Since a long time i prefer NUnit over XUnit. But i almost have forgotten how limited the attribute annotations of XUnit are in comparison to NUnit. You'll notice that when looking at those of my test cases being [Theory] and [InlineData] annotated in the test explorer in VS -- the displayed test case names are incredibly long and barely legible. With NUnit, that would have been a non-issue, as it allows you to specify a display name for every test case. But not so with XUnit. I am not going to do an (unrequested) migration of the test project from XUnit to NUnit just to be able write tests cases that can be identified with legible names. But i am also not going to spend time implementing silly workarounds in my test class for silly XUnit limitations. Well, i guess those test case names displayed in the test explorer and/or test logs are therefore what they are, until XUnit 3 will finally add a display name property to [InlineData], whenever that will be. At least the test code itself is readable already now... :-)

[JonathanMagnan]

Hello @rwecho ,

Just let me know when I can merge this pull request. From what I understand, you might want to integrate first the pull request of @elgonzo in your repository, then push it on this pull request.

[rwecho]

@elgonzo I reviewed the PR, it's very nice. I didn't expect that it's needed to test NodeReturnType.
And about the tests, I usually used XUnit and the long name is OK and readable.

@JonathanMagnan I have merged the PR. We can merge this PR now.

[elgonzo]

And about the tests, I usually used XUnit and the long name is OK and readable.

It's my attention span. I doze off reading halfway through such long names... 😆

Kidding aside, i just noticed that i should have added the new possible exception InvalidNodeReturnTypeException to the Intellisense comments of the GetEncapsulatedData method overloads. That should not hinder this PR being accepted, though. After this PR has been merged, i'll make another small PR adding the Intellisense comments. Sorry for the oversight...

[rwecho]

🤣 From I started programming, I knew the name of C# is usually longer than others.

    /// <summary>
        /// Fill an object and go through it's properties and fill them too.
        /// </summary>
        /// <typeparam name="T">Type of object to want to fill. It should have atleast one property that defined XPath.</typeparam>
        /// <returns>Returns an object of type T including Encapsulated data.</returns>
        /// <exception cref="ArgumentException">Why it's thrown.</exception>
        /// <exception cref="ArgumentNullException">Why it's thrown.</exception>
        /// <exception cref="MissingMethodException"><see cref="MissingMethodException"/></exception>
        /// <exception cref="MissingXPathException"><see cref="MissingXPathException"/></exception>
        /// <exception cref="XPathException"><see cref="XPathExeption"/></exception>
        /// <exception cref="NodeNotFoundException"><see cref="NodeNotFoundException"/></exception>
        /// <exception cref="NodeAttributeNotFoundException"><see cref="NodeAttributeNotFoundException"/></exception>
        /// <exception cref="FormatException">Why it's thrown.</exception>
        /// <exception cref="Exception">Why it's thrown.</exception>
        /// <exception cref="InvalidNodeReturnTypeException"><see cref="InvalidNodeReturnTypeException"/></exception>
        public T GetEncapsulatedData<T>()
        {
            return (T)GetEncapsulatedData(typeof(T), null);
        }

Kidding aside, i just noticed that i should have added the new possible exception InvalidNodeReturnTypeException to the Intellisense comments of the GetEncapsulatedData method overloads. That should not hinder this PR being accepted, though. After this PR has been merged, i'll make another small PR adding the Intellisense comments. Sorry for the oversight...

The exception you mentioned, is like the above code. I don't know how to describe them and just adding a mark is that OK?

[rwecho]

@JonathanMagnan This PR is ready to merge. If there is any other question about it. We can create a new PR to resolve.
@elgonzo It's OK?

[elgonzo]

@rwecho from my side it's alright. Thanks by the way for adding the Intellisense comments!