Doclet Type detection fix + tests #202

Redth · 2017-10-26T16:30:09Z

This fixes #201 a bit differently, and adds some tests for it.

Previously detection for _ApiXml doclet type was added but without consideration to the fact that `GetDocletType` really expects a directory and not a *file* path. This fixes things so that a file path can be passed in without a) causing an exception when the ‘index.html’ is appended to the file path and attempted to be read as a file, and b) won’t cause an exception if a directory is passed and tried to be opened as a file.

The `DocletType` property was used as a way to *set* which doclet type to use, but if it was null, and a doclet type was detected via the passed path in `CreateDocScraper`, we never assigned the resolved doclet type back to the property. This is useful primarily in creating tests, but arguably how it should have worked in the first place.

This adds a helper to the fixture to get a test resource and save its contents to a temp file.

Takes a path and expected doclet type and asserts they match.

This also switches to use the `LoadToTempFile` fixture helper instead of manually getting a temp path and loading the file.

We could use more for Java Doc types, but at least now this ensures the GetDocletType can handle both directories and paths.

jonathanpeppers

Tests passed for me local:

make run-tests TESTS=bin/TestDebug/Xamarin.Android.Tools.Bytecode-Tests.dll ANDROID_SDK_PATH=~/android-toolchain/sdk/
...
Tests run: 25, Errors: 0, Failures: 0, Inconclusive: 0, Time: 0.2511202 seconds

jonpryor · 2017-10-26T17:53:33Z

src/Xamarin.Android.Tools.Bytecode/ClassPath.cs

+			if (!DocletType.HasValue)
+				DocletType = GetDocletType(src);
+
+			switch (DocletType) {


Not exactly relevant to your patch, but given that CreateDocScraper() can be called multiple times, for multiple paths -- once per value in ClassPath.DocumentationPaths -- then I don't think DocletType should be used at all. What happens if/when ClassPath.DocumentationPaths contains multiple different types: the first one "wins"?

This should probably just be:

switch (GetDocletType (src)) {

Any objection to making GetDocletType public then? need some way to surface this code to tests.

So, from what I can tell, the intention behind DocletType in the first place was to allow classe-parse.exe to override the automatic detection. This property gets set if you invoke said exe with --docstype=...

So I guess we should leave this behaviour as is (leave the switch (DocletType ?? GetDocletType (..))) and just make public the GetDocletType method.

What's troubling about this statement is that class-parse always sets ClassPath.DocletType. docsType is a JavaDocletType, not a JavaDocletType?, so if class-parse --docstype isn't used, then ClassPath.DocletType will be set to JavaDocletType.DroidDoc, which is almost certainly wrong.

I'm starting to think we should [Obsolete] the ClassPath.DocletType property, and make it an error to use it entirely. :-/

On that matter, we could pretty much do away with all the various <ItemGroup> names currently out there and just use a <JavaParameterNameDocsPath .. /> or something similar since ClassPath already does the work of checking. In fact, the ClassParse task just concatenates all of these different items into a single set of doc paths: https://github.com/xamarin/xamarin-android/blob/master/src/Xamarin.Android.Build.Tasks/Tasks/ClassParse.cs#L36-L41

So, if we obsolete ClassPath.DocletType we should also make it an error to specify --docstype= in class-parse.exe.

This might also explain some weirdness i've seen in getting the generator to parse parameter names for the android support library bindings which are known to be in the documentation... Seems like maybe it's just getting detected as a DroidDoc always.

jonpryor · 2017-10-26T17:55:55Z

src/Xamarin.Android.Tools.Bytecode/Tests/ParameterFixupTests.cs

+				AssertXmlDeclaration ("Collection.class", "ParameterFixupFromDocs.xml", tempFile, Bytecode.JavaDocletType._ApiXml);
+			} catch (Exception ex) {
+				try {
+					if (File.Exists (tempFile))


This shouldn't be in the catch block, this should be in a finally block, so that we always delete tempFile. (Looks like this was a bug in the original code.)

jonpryor · 2017-10-26T17:56:52Z

src/Xamarin.Android.Tools.Bytecode/Tests/ParameterFixupTests.cs

+				}
+				catch { }
+
+				Assert.Fail ("An unexpected exception was thrown : {0}", ex);


...all the more reason to delete tempFile within a finally block instead of a catch block: this Assert wouldn't be needed anymore.

jonpryor · 2017-10-26T18:06:13Z

src/Xamarin.Android.Tools.Bytecode/Tests/ParameterFixupTests.cs

+				return;
+			}
+
+			try {


Do we actually need this try/catch block here?

class-parse.exe expects to be able to set the DocletType property and override all invocations to GetDocletType, so reverting this in the spirit of not changing expected behaviour there.

GetDocletType is made public to invoke from tests. There was no other visible way to test the doclet type resolution/detection as `DocletType` property is never assigned any value from this detection and only intended to be _set_.

This test should not be explicitly setting doctype, it should be determined by ClassPath instead.

jonpryor · 2017-10-26T18:38:13Z

src/Xamarin.Android.Tools.Bytecode/Tests/ParameterFixupTests.cs

+			} finally {
+				try {
+					if (File.Exists (tempFile))
+						File.Delete (tempFile);


We shouldn't need to wrap this in try/catch.

jonpryor · 2017-10-26T18:38:35Z

src/Xamarin.Android.Tools.Bytecode/Tests/ParameterFixupTests.cs

+			} finally {
+				try {
+					if (File.Exists (tempFile))
+						File.Delete (tempFile);


This shouldn't need to be wrapped in try/catch.

jonpryor · 2017-10-26T18:42:03Z

src/Xamarin.Android.Tools.Bytecode/ClassPath.cs

 		}

-		JavaDocletType GetDocletType (string path)
+		public JavaDocletType GetDocletType (string path)


This should be made static, and moved to AndroidDocScraper.

This also does away with the `AssertDocletType` helper since the tests can now do the same directly in one line of code.

These temp files should really not be ever accessed by anything else, so there should be no issue calling .Delete on them if they exist.

jonpryor · 2017-10-27T00:10:32Z

src/Xamarin.Android.Tools.Bytecode/JavaDocumentScraper.cs

+
+			string indexHtml = Path.Combine (path, "index.html");
+			if (File.Exists (indexHtml))
+			{


You have some curious formatting settings applied. You have space before ( (yay), but { is on a new line (?).

That's...unexpectedly bizarre.

Please place the { on the same line as the if. :-)

Currently the default is set to 0 (so `DroidDoc`), so there is a value set even if the `—docstype` isn’t specified as a parameter. Since `ClassPath` is [always instantiated](https://github.com/xamarin/java.interop/blob/master/tools/class-parse/Program.cs#L74) with `DocletType` assigned to this value, when `CreateDocScraper` [gets invoked](https://github.com/xamarin/java.interop/blob/master/src/Xamarin.Android.Tools.Bytecode/ClassPath.cs#L267) it is always using the `DroidDoc` value instead of automatically detecting the doc type. This small change should cause automatic resolution of doc type to happen if `class-parse.exe` is invoked without the `—docstype` arg.

jonpryor · 2017-10-27T15:05:58Z

src/Xamarin.Android.Tools.Bytecode/ClassPath.cs

 		IAndroidDocScraper CreateDocScraper (string src)
 		{
-			switch (DocletType ?? GetDocletType (src)) {
+			switch (DocletType ?? AndroidDocScraper.GetDocletType (src)) {


It's good that we updated class-parse so that it doesn't always set DocletType, but...do we really want to prefer it?

I'm not convinced that we do.

dnfclas added the cla-already-signed label Oct 26, 2017

Redth added 6 commits October 26, 2017 12:35

Bytecode Tests - Load resource to temp file helper

97031e6

This adds a helper to the fixture to get a test resource and save its contents to a temp file.

Bytecode Tests - added doclet type assertion

675ed94

Takes a path and expected doclet type and asserts they match.

Bytecode Tests - fix code formatting on old test

60f154e

This also switches to use the `LoadToTempFile` fixture helper instead of manually getting a temp path and loading the file.

Bytecode Tests - add doclet type resolution tests

99b4e87

We could use more for Java Doc types, but at least now this ensures the GetDocletType can handle both directories and paths.

Redth force-pushed the doclettype-fix-tests branch from 6fc68b2 to 99b4e87 Compare October 26, 2017 16:36

jonathanpeppers requested review from jonathanpeppers and jonpryor October 26, 2017 16:42

jonathanpeppers approved these changes Oct 26, 2017

View reviewed changes

jonpryor reviewed Oct 26, 2017

View reviewed changes

jonpryor mentioned this pull request Oct 26, 2017

[class-parse] add support for external parameter name descriptor files. #200

Closed

jonpryor reviewed Oct 26, 2017

View reviewed changes

Redth added 5 commits October 26, 2017 14:22

Revert back to DocletType ?? GetDocletType (..)

3538b7a

class-parse.exe expects to be able to set the DocletType property and override all invocations to GetDocletType, so reverting this in the spirit of not changing expected behaviour there.

Change DocletType Assertion, invoke GetDocletType

fde965e

GetDocletType is made public to invoke from tests. There was no other visible way to test the doclet type resolution/detection as `DocletType` property is never assigned any value from this detection and only intended to be _set_.

Bytecode tests - delete temp file in finally

d2d85a3

Bytecode tests - doc type is implicit

8301bf6

This test should not be explicitly setting doctype, it should be determined by ClassPath instead.

Bytecode tests - remove unnecssary try/catch

21e82d3

jonpryor reviewed Oct 26, 2017

View reviewed changes

Redth added 2 commits October 26, 2017 15:02

Move GetDocletType to AndroidDocScraper

f050780

This also does away with the `AssertDocletType` helper since the tests can now do the same directly in one line of code.

Bytecode Tests - remove unnecessary try/catch

fef5efe

These temp files should really not be ever accessed by anything else, so there should be no issue calling .Delete on them if they exist.

jonpryor reviewed Oct 27, 2017

View reviewed changes

Redth added 2 commits October 26, 2017 21:29

Fix formatting { got autoformatted to new line

b975f8e

jonpryor reviewed Oct 27, 2017

View reviewed changes

Remove DocletType and auto detect always

937297a

jonpryor merged commit 59a90c4 into dotnet:master Oct 27, 2017

github-actions bot locked and limited conversation to collaborators Apr 15, 2024

+              				return;
+              			}
+              			try {

Doclet Type detection fix + tests #202

Doclet Type detection fix + tests #202

Uh oh!

Conversation

Redth commented Oct 26, 2017

Uh oh!

jonathanpeppers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants