TRUNK-4870 Add Unit tests for ModuleUtil.expandJar(File, File, Stri… #1866

mjanuchowski · 2016-11-04T18:32:27Z

Description

created 5 unit tests for expandJar function
added 1 simplified test jar file used by unit tests
created 5 @should annotations in expandJar header
one of the test cases was failing (name parameter is a file, keepFullPath is false), had to do change to expandJar function to fix a bug

Related Issue

https://issues.openmrs.org/browse/TRUNK-4870

Checklist:

My pull request only contains one single commit.
My pull request is based on the latest master branch
git pull --rebase upstream master.
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.
My code follows the code style of this project.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

mention-bot · 2016-11-04T18:32:29Z

@mjanuchowski, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bmamlin, @teleivo and @dkayiwa to be potential reviewers.

coveralls · 2016-11-04T18:40:47Z

Coverage increased (+0.01%) to 55.722% when pulling 2803f67 on mjanuchowski:TRUNK-4870 into ef0d944 on openmrs:master.

teleivo

I suggest tests should

follow the new style of unit tests, use
https://wiki.openmrs.org/display/docs/Generate+Test+Case+Plugin
make tests cleaner (use javas File type without doing all the string concatenation with File.separator and string replace); added a sample of how I would rewrite one of the written tests

I think the change in the implementation shouldnt have the separator hard coded

teleivo · 2016-11-04T19:11:52Z

api/src/test/java/org/openmrs/module/ModuleUtilTest.java

+	 */
+	@Test
+	@Verifies(value = "expand entire jar if name is null", method = "expandJar(File,File,String,boolean)")
+	public void expandJar_shouldExpandEntireJarIfNameIsNull() throws IOException {


Can you please use the openmrs generate test case plugin, see https://wiki.openmrs.org/display/docs/Generate+Test+Case+Plugin

You used the old test style and the generator will easily create the scaffold for you in the new style :)

teleivo · 2016-11-04T19:30:45Z

api/src/main/java/org/openmrs/module/ModuleUtil.java

@@ -554,6 +554,11 @@ public static URL file2url(final File file) throws MalformedURLException {
 	 * @param keepFullPath if true, will recreate entire directory structure in tmpModuleDir
 	 *            relating to <code>name</code>. if false will start directory structure at
 	 *            <code>name</code>
+	 * @should expand entire jar if name is null
+	 * @should expand directory with parent tree if name is directory and keepFullPath is true


can you please add a test for when name is an empty string. wonder what happens :)

teleivo · 2016-11-04T19:37:26Z

api/src/test/java/org/openmrs/module/ModuleUtilTest.java

+	@Test
+	@Verifies(value = "expand entire jar if name is null", method = "expandJar(File,File,String,boolean)")
+	public void expandJar_shouldExpandEntireJarIfNameIsNull() throws IOException {
+		// Arrange


comments like arrange, act, assert, clean up. are not used in openmrs tests, as far as i know. can you just replace them with newlines.

I find them helpful to keep tests clean, but no problem, I will apply to rules that are used in a project :)

teleivo · 2016-11-04T19:54:28Z

api/src/test/java/org/openmrs/module/ModuleUtilTest.java

+		// Act
+		ModuleUtil.expandJar(getJarFile(), destinationFolder, directoryPath, true);
+		// Assert
+		Object[] filesList = FileUtils.listFiles(destinationFolder, null, true).toArray();


what about using type File instead of Object you then dont need to cast multiple times later on.

or maybe use a list:
List filesList = (List) FileUtils.listFiles(destinationFolder, null, true);

teleivo · 2016-11-04T20:35:59Z

api/src/test/java/org/openmrs/module/ModuleUtilTest.java

+		String ActualPath = ((File)filesList[0]).getPath().replace(((File)filesList[0]).getName(), "");
+		Assert.assertEquals(expectedPath, ActualPath);
+		// Clean up
+		FileUtils.deleteDirectory(destinationFolder);


i think you can simplify your tests by leveraging the java File you dont need to use/add/replace file separators.

I think you can do something like

public void expandJar_shouldExpandDirectoryWithoutParentTreeIfNameIsDirectoryAndKeepFullPathIsFalse() throws IOException { final int numberOfFilesInSpecifiedDirectory = 2; String directoryPath = "META-INF/maven/org.openmrs.module/test1-api"; File destinationFolder = this.getEmptyJarDestinationFolder(); ModuleUtil.expandJar(getJarFile(), destinationFolder, directoryPath, false); List<File> actualExpandedFiles = (List<File>) FileUtils.listFiles(destinationFolder, null, true); Assert.assertEquals(numberOfFilesInSpecifiedDirectory, actualExpandedFiles.size()); Assert.assertEquals(destinationFolder.toString(), actualExpandedFiles.get(0).getParent()); FileUtils.deleteDirectory(destinationFolder); }

teleivo · 2016-11-04T20:40:20Z

api/src/main/java/org/openmrs/module/ModuleUtil.java

@@ -571,7 +576,15 @@ public static void expandJar(File fileToExpand, File tmpModuleDir, String name,
 					String entryName = jarEntry.getName();
 					// trim out the name path from the name of the new file
 					if (!keepFullPath && name != null) {
-						entryName = entryName.replaceFirst(name, "");
+						if (name.equals(entryName)) {
+							int lastSlash = name.lastIndexOf('/');


you use File.separator in your tests but hard code the separator here, why?

Since JarEntry uses slash as a separator I would expect that name parameter will also use them. This is in line with rest of the code in this function, which also uses hard coded slash.

Are you saying jarEntry uses forward slash on other systems like windows?

Yes, I ran unit tests also on Windows and they do not fail, which proves that paths in JarEntry do not use File.Separator but always forward slash.

coveralls · 2016-11-07T14:47:17Z

Coverage increased (+0.02%) to 55.632% when pulling 5b039ef on mjanuchowski:TRUNK-4870 into 17c22ce on openmrs:master.

teleivo · 2016-11-07T15:11:35Z

@mjanuchowski thanks for the making the changes 👍

wluyima · 2016-11-07T21:37:54Z

api/src/main/java/org/openmrs/module/ModuleUtil.java

+						if (name.equals(entryName)) {
+							int lastSlash = name.lastIndexOf('/');
+							if (lastSlash >= 0) {
+								entryName = entryName.replaceFirst(name.substring(0, lastSlash + 1), "");


Can you please add a comment here to explain what you're trying to solve?

@wluyima what about extracting it into a private method with a descriptive name instead of a comment? A lot of methods are already bloated and hard to read since they do so many things

Which ever way he does it, as long as it's clear to the reader I'd be okay with it

Actually if block in which this code is nested has already a pretty good description:
// trim out the name path from the name of the new file
I'm just making sure that we trim only a path, without the file name itself.

That comment made sense for the old changes though, it was trimming out just the name, yours seems to have an extra uncommented inner if clause that's not clear what it does when one glances at the code

This comment is still valid - the whole block trims out the name path from the name of the new file. Code added in inner if statement makes sure that it happens no matter if name parameter is file or directory. But of course I can add comments to make that clear :)

That's what I've been trying to ask for :), i.e to add the comment to say why you have that inner clause and logic, thanks!

coveralls · 2016-11-07T23:33:56Z

Coverage increased (+0.01%) to 55.623% when pulling 3ae01a7 on mjanuchowski:TRUNK-4870 into 17c22ce on openmrs:master.

mjanuchowski · 2016-11-10T11:50:07Z

Thank you for all the suggestions! Is there any extra work on this ticket?

wluyima · 2016-11-11T15:41:07Z

api/src/main/java/org/openmrs/module/ModuleUtil.java

+						if (name.equals(entryName)) {
+							int lastSlash = name.lastIndexOf('/');
+							if (lastSlash >= 0) {
+								entryName = entryName.replaceFirst(name.substring(0, lastSlash + 1), "");


I'm still a little confused by this change, which test fails when you remove this? The old logic trims out the filename from the entry so that you remain with the path but it appears to me like you're doing the opposite here i.e you are stripping out the path and remaining with the filename

The failing test was expanding file without full path: expandJar_shouldExpandFileWithoutParentTreeIfNameIsFileAndKeepFullPathIsFalse(). In this case name provides filename with path e.g. FOLDER1/FOLDER2/FILE. Original function was stripping this whole string from jar filename which was leaving it empty. As a result nothing was getting extracted. What I did is to make sure in that specific case that only path part of name gets trimmed out, so that jar filename still stays and gets extracted.

I understand it when you say that but do we actually ever call this method with a path and not a filename?

You're right, in openmrs-core this function is always called with name parameter as filename, but at the same time it is always called with keepFullPath == true, so the case causing an error never occured.

I guess what am saying is that it was intentional to exclude this, right now it just seems to me like a contrived case.

I'm not sure if I understand. Do you mean that keepFullPath parameter is obsolete and full path should be always recreated? But isn't it possible that this function gets called from outside of openmrs-core or gets used in future with keepFullPath == false?
What I did is, since it has keepFullPath and name parameters, make sure that it works with every combination of those two.

What am saying is this code is intended to be called by the module framework in core only and if it always calls the method with keepFullPath set to true and name as a filename, then we don't need this change

Ok, I can revert the change and get rid of failing test.
Still I would prefer such function to work properly with all parameter combinations and not only with those that we can observe in a present code. :)

I'm trying to go by this saying don't try to fix it if ain't broke, until something fails because this code is missing then we will fix it at that point, as long as all reference to it in core set the parameters to true and the filename respectively and it works, it's all good otherwise you risk introducing other obscure bugs, we don't expect external code to be calling this method.

Is there any extra work on this ticket?

lluismf · 2016-11-11T18:02:16Z

Check out Apache FilenameUtils, it contains a lot of handy methods to deal with file names (maybe you won't need any of it)

coveralls · 2016-11-14T20:46:00Z

Coverage increased (+0.006%) to 55.74% when pulling 1fc2dbc on mjanuchowski:TRUNK-4870 into de99ca9 on openmrs:master.

dkayiwa · 2016-11-20T12:09:33Z

api/src/test/java/org/openmrs/module/ModuleUtilTest.java

+	 * @return <code>File</code> containing folder for Jar tests.
+	 */
+	protected File getEmptyJarDestinationFolder() throws IOException {
+		File destinationFolder = new File("/tmp/expandedJar");


Does the path above work on windows? :)

Yes it does :) I've ran test on both Linux and Windows.

I would need another windows user to run and confirm it. :)
Why don't you use System.getProperty("java.io.tmpdir")?

Good idea! I will use it.

…ng, boolean)

coveralls · 2016-11-21T00:57:43Z

Coverage increased (+0.03%) to 55.736% when pulling d589443 on mjanuchowski:TRUNK-4870 into 7ce4dfa on openmrs:master.

teleivo requested changes Nov 4, 2016

View reviewed changes

mjanuchowski force-pushed the TRUNK-4870 branch from 2803f67 to 5b039ef Compare November 7, 2016 14:36

wluyima reviewed Nov 7, 2016

View reviewed changes

mjanuchowski force-pushed the TRUNK-4870 branch from 5b039ef to 3ae01a7 Compare November 7, 2016 23:13

wluyima reviewed Nov 11, 2016

View reviewed changes

mjanuchowski force-pushed the TRUNK-4870 branch from 3ae01a7 to 1fc2dbc Compare November 14, 2016 20:40

dkayiwa reviewed Nov 20, 2016

View reviewed changes

TRUNK-4870 - Add Unit tests for ModuleUtil.expandJar(File, File, Stri…

d589443

…ng, boolean)

mjanuchowski force-pushed the TRUNK-4870 branch from 1fc2dbc to d589443 Compare November 21, 2016 00:52

dkayiwa merged commit e4d2ea9 into openmrs:master Nov 21, 2016

mjanuchowski deleted the TRUNK-4870 branch November 30, 2016 14:44

TRUNK-4870 Add Unit tests for ModuleUtil.expandJar(File, File, Stri… #1866

TRUNK-4870 Add Unit tests for ModuleUtil.expandJar(File, File, Stri… #1866

Conversation

mjanuchowski commented Nov 4, 2016

Description

Related Issue

Checklist:

mention-bot commented Nov 4, 2016

coveralls commented Nov 4, 2016

teleivo left a comment • edited Loading

Choose a reason for hiding this comment

teleivo Nov 4, 2016 • 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

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

coveralls commented Nov 7, 2016

teleivo commented Nov 7, 2016

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

wluyima Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

coveralls commented Nov 7, 2016

mjanuchowski commented Nov 10, 2016

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

Choose a reason for hiding this comment

mjanuchowski Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

wluyima Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lluismf commented Nov 11, 2016

coveralls commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjanuchowski Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

coveralls commented Nov 21, 2016

teleivo left a comment •

edited

Loading

teleivo Nov 4, 2016 •

edited

Loading

wluyima Nov 7, 2016 •

edited

Loading

mjanuchowski Nov 14, 2016 •

edited

Loading

wluyima Nov 14, 2016 •

edited

Loading

mjanuchowski Nov 21, 2016 •

edited

Loading