Issue #6232: Load resources relative to resources root #6600

peterdemaeyer · 2019-03-21T21:02:02Z

Added new unit tests to illustrate problem - test fails.
Fixed code - test succeeds.
Adapted existing unit test for new behavior.
"mvn clean verify" succeeded.

rnveach · 2019-03-21T23:21:14Z

@peterdemaeyer spellchecker in travis is failed and pitest is failing in circle ci.
Please keep number of commits at 1 while you make CI pass.

rnveach · 2019-03-27T01:11:41Z

@peterdemaeyer pitest is still failing.

peterdemaeyer · 2019-03-28T07:36:32Z

The build timed out while downloading dependencies. Can someone please rebuild to see if it's maybe a transient issue? Apparently I don't have permission to trigger a rebuild myself.

rnveach · 2019-03-28T11:50:27Z

I restarted the CI.

peterdemaeyer · 2019-03-29T20:49:48Z

Thanks, all green now.

src/test/java/com/puppycrawl/tools/checkstyle/utils/CommonUtilTest.java

romani · 2019-04-17T11:10:23Z

@peterdemaeyer , thanks a lot for your updates.
FYI, if no activity till end of month we will close PR (just to keep out list of PRs manageable), you are welcome to reopen it ones you have new updates and ready to continue.

peterdemaeyer · 2019-04-27T16:47:03Z

Fixed the review comments, but now the wercker/build fails with an unclear "401". mvn clean verify succeeds locally.

romani · 2019-04-27T16:47:19Z

@rnveach , please continue/finish review.

rnveach · 2019-04-27T16:59:45Z

@peterdemaeyer failure is because your branch is based on an outdated master. Please rebase.

rnveach · 2019-04-27T17:02:00Z

src/test/resources/commonutil/InputCommonUtilTest_resource.txt

@@ -0,0 +1 @@
+good


Why does this file need to be in its own folder and not part of checkstyle's standard packaging?

Because it most elegantly illustrates the initial issue I encountered and have now fixed - which is to reach for a file outside of the package you're currently in.
I could move everything inside the "standard packaging", but at the expense of adding an excessively deep directory tree.
The "good" file would be then in src/test/resources/com/puppycrawl/tools/checkstyle/utils/commonutil, the "bad" file would be in src/test/resources/com/puppycrawl/tools/checkstyle/utils/commonutil/com/puppycrawl/tools/checkstyle/utils/commonutil, which would be a lot less comprehensive and a less good representation of what I'm trying to illustrate.

@romani What do you think of this?

I could move everything inside the "standard packaging", but at the expense of adding an excessively deep directory tree.

please deep path. Nobody will look at it, till deal with certain test.

This is not so easy to fix after all. A good test must first fail without the fix (illustrating the issue) and succeed with the fix. The easy part is to make the test succeed with the fix. The hard part is to make it fail without the fix. That's why I have the two files by the way, the "good" file and the "bad" file. Whatever I try, there is always a combination where one of these files causes the AllTestsTest to fail. That is the reason it's taking me so long to fix this.

I tried, but I couldn't move the resources while still keeping AllTestsTest happy, and I certainly don't want to reduce the value of my test, so I left it the way it was because it's the only way I could preserve the value of the test while still satisfying AllTestsTest.

@peterdemaeyer what failure were you getting in AllTestsTest and what path were you setting it to?
The only reason AllTestsTest isn't flagging this now is because it is specifically looking in com/puppycrawl sub-folder which your file is not in.
You can add suppression to test and we can review later.

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/internal/AllTestsTest.java

Line 153 in a73ff08

if (!path.contains(File.separatorChar + "grammar" + File.separatorChar)

checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/internal/AllTestsTest.java

Line 255 in a73ff08

private static boolean shouldSkipInputFileNameCheck(String path, String fileName) {

src/test/java/com/puppycrawl/tools/checkstyle/utils/CommonUtilTest.java

peterdemaeyer · 2019-04-28T13:16:25Z

The build fails because of what I believe to be an issue with the build system: it seems that openjdk 9/10/11/12 just fails to install on it.

rnveach

Sorry, 1 final change.

...ources/com/puppycrawl/tools/checkstyle/utils/commonutil/InputCommonUtilTest_empty_checks.xml

romani · 2019-05-17T05:25:24Z

@peterdemaeyer , ping.
It would be awesome to finish this PR

peterdemaeyer · 2019-05-17T14:58:30Z

Yeah, sorry, I left this aside a bit after I ran into some trouble moving the resources around in an attempt to satisfy one of your remarks. I'll try picking it up again this weekend.

romani · 2019-05-18T22:38:24Z

If problem is heavy .... please share it, we can reconsider decisions.

peterdemaeyer · 2019-05-19T08:47:58Z

The remaining "problem" is the one about the test resources: the location of "good" and "bad" versions of InputCommonUtilTest_resources.txt. I thought I could move them to a subfolder, but then I hit some assertion of AllTestsTest. In the end, I didn't find a way to satisfy AllTestsTest without reducing the value of my test ("the value of my test" = not only does it have to succeed with the fix, it also has to fail for the right reason without the fix). Because I certainly don't want to reduce the value of my test, I just left it the way it was.

Additionally, I now run into a build issue I didn't have before, I still need to investigate that.

romani

item to improve:

romani · 2019-05-19T14:15:52Z

src/test/java/com/puppycrawl/tools/checkstyle/utils/CommonUtilTest.java

+    }
+
+    @Override
+    protected String getPackageLocation() {


please move this method to the top of class, right after fields.

It better to reply "done" when you finished review item.

OK - I'll do that.
I expected the button "Resolve conversation" to do more or less the same, but apparently not (not sure what it actually does then).

I expected the button "Resolve conversation" to do more or less the same, but apparently not

We don't get notifications on clicking resolve and pushing commits, so it could go unnotice that you made changes. Also we ask that we be the ones to click resolve conversation so we can use it as a way to double verify the work.

romani · 2019-05-23T01:46:44Z

I hit some assertion of AllTestsTest

Please share error message.

peterdemaeyer · 2019-05-25T18:05:01Z

I finally managed to move the resource, and the AllTestsTest passes.

peterdemaeyer · 2019-05-26T16:02:46Z

The build is stuck waiting for a build slave.

rnveach · 2019-05-28T11:56:06Z

Jenkins can be ignored.

rnveach · 2019-05-28T11:58:50Z

I finally managed to move the resource, and the AllTestsTest passes.

"com/puppycrawl/tools/checkstyle/utils/"
+ "com/puppycrawl/tools/checkstyle/utils/"

Why the double package in the folder path?

romani · 2019-06-01T04:17:45Z

@peterdemaeyer ,
Please answer question from @rnveach .

romani

items to improve:

romani · 2019-06-01T12:47:38Z

src/test/java/com/puppycrawl/tools/checkstyle/utils/CommonUtilTest.java

+    @Test
+    public void testGetUriByFilenameFindsResourceRelativeToRootClasspath() throws Exception {
+        final String filename =
+                "com/puppycrawl/tools/checkstyle/utils/commonutil/InputCommonUtilTest_resource.txt";


getPackageLocation() + "/InputCommonUtilTest_resource.txt"

Consider it done (commit pending).

romani · 2019-06-01T12:50:43Z

src/test/java/com/puppycrawl/tools/checkstyle/utils/CommonUtilTest.java

+        final String uriRelativeToPackage =
+                "com/puppycrawl/tools/checkstyle/utils/"
+                        + "com/puppycrawl/tools/checkstyle/utils/"
+                        + "commonutil/InputCommonUtilTest_resource.txt";


getPackageLocation() + "/InputCommonUtilTest_resource.txt"

Consider it done (commit pending).

romani · 2019-06-01T12:55:12Z

src/test/java/com/puppycrawl/tools/checkstyle/utils/CommonUtilTest.java

+                        + "com/puppycrawl/tools/checkstyle/utils/"
+                        + "commonutil/InputCommonUtilTest_resource.txt";
+        assertThat("URI is relative to package " + uriRelativeToPackage,
+            uri.toString(), not(containsString(uriRelativeToPackage)));


please explain this assert , filename and uriRelativeToPackage are both relative . uriRelativeToPackage is bigger than filename so it will always have no assert. Do I miss something ?
We probably need some comments over code, as it is not obvious, or improve names of variables.

I think we need some summary from #6600 (comment)
with wording something like:

this test case is for "Issue #6232" without fix if user define "com/puppycrawl/tools/checkstyle/utils/commonutil/InputCommonUtilTest_resource.txt" it is was converted to "com/puppycrawl/tools/checkstyle/utils" + "/" + "com/puppycrawl/tools/checkstyle/utils/commonutil/InputCommonUtilTest_resource.txt"

OK, I'll explain everything in a Javadoc comment on the test method.
The assertion asserts that the URL does not contain the excessively deep path "com/puppycrawl/tools/checkstyle/utils/com/puppycrawl/tools/checkstyle/utils/commonutil".
It does fail without the fix.
With the fix, the location is not excessively deep, and the assertion succeeds.
Note that replacing the relevant portions of the strings with getPackageLocation() may make the test even less obvious, but I'll do it anyway.
Consider it done (commit pending).

romani · 2019-06-01T12:57:04Z

src/test/java/com/puppycrawl/tools/checkstyle/utils/CommonUtilTest.java

+            IOUtils.copy(in, out);
+        }
+        assertThat("Content mismatches for: " + uri,
+            new String(out.toByteArray(), StandardCharsets.UTF_8), startsWith("good"));


String fileContent = new String(out.toByteArray(), StandardCharsets.UTF_8)
the simpler assertThat the better.

can we use https://github.com/apache/commons-io/blob/master/src/main/java/org/apache/commons/io/IOUtils.java#L2680 ? to skip all details on how actually content of file is loaded to String.

Good idea. Consider it done (commit pending).

peterdemaeyer · 2019-06-06T20:29:26Z

I finally managed to move the resource, and the AllTestsTest passes.

"com/puppycrawl/tools/checkstyle/utils/"
+ "com/puppycrawl/tools/checkstyle/utils/"

Why the double package in the folder path?

Because I want to illustrate that the "good" file is not in an excessively deep directory. It's only the "bad" file which is in the excessively deep directory.
Note that all this would have been easier to understand if I could have put the "good" file in the root (of the test resources) as I did initially. I was not too happy having to move them, precisely for this reason. Remember I said "at the cost of an excessively deep directory tree" - this is exactly what I meant.

I'll explain everything in a Javadoc comment on the test.

romani

Ok to merge, thanks a lot for your efforts to finish implementation

peterdemaeyer force-pushed the issue-6263 branch from 1779abe to 6de5e93 Compare March 25, 2019 20:33

peterdemaeyer force-pushed the issue-6263 branch from 6de5e93 to 94e787b Compare March 27, 2019 20:42

rnveach requested changes Apr 1, 2019

View reviewed changes

rnveach self-assigned this Apr 1, 2019

romani added the awaiting changes label Apr 1, 2019

peterdemaeyer force-pushed the issue-6263 branch from 94e787b to 734967c Compare April 27, 2019 16:26

romani removed the awaiting changes label Apr 27, 2019

rnveach reviewed Apr 27, 2019

View reviewed changes

peterdemaeyer force-pushed the issue-6263 branch 2 times, most recently from eacd920 to ff27f0d Compare April 28, 2019 08:24

rnveach requested changes Apr 29, 2019

View reviewed changes

...ources/com/puppycrawl/tools/checkstyle/utils/commonutil/InputCommonUtilTest_empty_checks.xml Outdated Show resolved Hide resolved

peterdemaeyer force-pushed the issue-6263 branch 2 times, most recently from a73a6e8 to d78e355 Compare May 19, 2019 08:22

peterdemaeyer force-pushed the issue-6263 branch from d78e355 to c2f61af Compare May 19, 2019 13:53

romani requested changes May 19, 2019

View reviewed changes

peterdemaeyer force-pushed the issue-6263 branch from c2f61af to eb29012 Compare May 19, 2019 14:21

peterdemaeyer force-pushed the issue-6263 branch 2 times, most recently from 9088073 to 8bc51a0 Compare May 25, 2019 18:01

peterdemaeyer force-pushed the issue-6263 branch from 8bc51a0 to 301a511 Compare May 25, 2019 18:47

rnveach approved these changes May 28, 2019

View reviewed changes

rnveach assigned romani and unassigned rnveach May 28, 2019

romani requested changes Jun 1, 2019

View reviewed changes

peterdemaeyer force-pushed the issue-6263 branch from 301a511 to 682fa6d Compare June 6, 2019 20:33

Issue checkstyle#6232: Load resources relative to resources root

52690ff

peterdemaeyer force-pushed the issue-6263 branch from 682fa6d to 52690ff Compare June 6, 2019 20:34

romani approved these changes Jun 7, 2019

View reviewed changes

romani merged commit 9b8dea4 into checkstyle:master Jun 7, 2019

Issue #6232: Load resources relative to resources root #6600

Issue #6232: Load resources relative to resources root #6600

Conversation

peterdemaeyer commented Mar 21, 2019 • edited by rnveach Loading

rnveach commented Mar 21, 2019

rnveach commented Mar 27, 2019

peterdemaeyer commented Mar 28, 2019

rnveach commented Mar 28, 2019

peterdemaeyer commented Mar 29, 2019

romani commented Apr 17, 2019

peterdemaeyer commented Apr 27, 2019

romani commented Apr 27, 2019

rnveach commented Apr 27, 2019

Choose a reason for hiding this comment

peterdemaeyer Apr 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterdemaeyer commented Apr 28, 2019

rnveach left a comment

Choose a reason for hiding this comment

romani commented May 17, 2019

peterdemaeyer commented May 17, 2019

romani commented May 18, 2019

peterdemaeyer commented May 19, 2019

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani commented May 23, 2019

peterdemaeyer commented May 25, 2019

peterdemaeyer commented May 26, 2019

rnveach commented May 28, 2019

rnveach commented May 28, 2019 • edited Loading

romani commented Jun 1, 2019

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani Jun 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterdemaeyer commented Jun 6, 2019

romani left a comment

Choose a reason for hiding this comment

peterdemaeyer commented Mar 21, 2019 •

edited by rnveach

Loading

peterdemaeyer Apr 27, 2019 •

edited

Loading

rnveach commented May 28, 2019 •

edited

Loading

romani Jun 1, 2019 •

edited

Loading