Skip to content
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.

Sign up for GitHub

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

Add plugin information for Verbose mode #18051

Merged
merged 8 commits into from
May 10, 2016
Merged

Add plugin information for Verbose mode #18051

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
397c9f1
Add plugin information for Verbose mode
gmoskovicz Apr 28, 2016
8307a05
Fixed code review comments and added tests.
gmoskovicz Apr 29, 2016
f94a749
Added helper on another test.
gmoskovicz Apr 29, 2016
fb2c1b7
Added stronger assertions.
gmoskovicz Apr 29, 2016
234bc68
Fixed and simplify tests
gmoskovicz May 2, 2016
ffe41ec
Fixed tests with latest comments
gmoskovicz May 6, 2016
206d2f9
Fixed no-needed brackets and long line
gmoskovicz May 6, 2016
9f87f9f
Fix indentation.
gmoskovicz May 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions core/src/main/java/org/elasticsearch/plugins/ListPluginsCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,18 @@ protected void execute(Terminal terminal, OptionSet options) throws Exception {
try (DirectoryStream<Path> stream = Files.newDirectoryStream(env.pluginsFile())) {
for (Path plugin : stream) {
terminal.println(plugin.getFileName().toString());
PluginInfo info = null;
try {
info = PluginInfo.readFromProperties(env.pluginsFile().resolve(plugin.toAbsolutePath()));
terminal.println(Terminal.Verbosity.VERBOSE, info.toString());
}
catch(IOException ex) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also is not necessary, I think the file not found is explicit enough (and no different than a user would see when actually starting elasticsearch).

Copy link
Member

@jasontedor jasontedor Apr 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't think we need a separate catch here. Also, not all IOException are because of file not found.

terminal.println(Terminal.Verbosity.VERBOSE, "Plugin properties file missing for plugin");
}
catch (Exception ex){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think we should add leniency here. This will only hide the actual problem. Let the exception propagate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exception should immediately bubble up to the user. Catch it, add which plugin it died on with this as the cause and throw.

terminal.println(Terminal.Verbosity.VERBOSE, "Unable to retrieve plugin information."
+ " Please verify that this plugin is compatible with your current version");
}
}
}
}
Expand Down
44 changes: 43 additions & 1 deletion qa/evil-tests/src/test/java/org/elasticsearch/plugins/ListPluginsCommandTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,19 @@
package org.elasticsearch.plugins;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs a test for the exceptional case (exception during reading).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.List;

import org.apache.lucene.util.LuceneTestCase;
import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.MockTerminal;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.env.Environment;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.Version;

@LuceneTestCase.SuppressFileSystems("*")
public class ListPluginsCommandTests extends ESTestCase {
Expand All @@ -43,13 +47,17 @@ Environment createEnv() throws IOException {
}

static MockTerminal listPlugins(Environment env) throws Exception {
return listPlugins(env, new String[0]);
}

static MockTerminal listPlugins(Environment env, String[] args) throws Exception {
MockTerminal terminal = new MockTerminal();
String[] args = {};
int status = new ListPluginsCommand(env).main(args, terminal);
assertEquals(ExitCodes.OK, status);
return terminal;
}


public void testPluginsDirMissing() throws Exception {
Environment env = createEnv();
Files.delete(env.pluginsFile());
Expand Down Expand Up @@ -80,4 +88,38 @@ public void testTwoPlugins() throws Exception {
assertTrue(output, output.contains("fake1"));
assertTrue(output, output.contains("fake2"));
}

public void testPluginWithVerbose() throws Exception {
Environment env = createEnv();
Files.createDirectory(env.pluginsFile().resolve("fake1"));
Path configFile = Files.createFile(env.pluginsFile().resolve("fake1").resolve(PluginInfo.ES_PLUGIN_PROPERTIES));
List<String> config = Arrays.asList("description=fake plugin",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a test helper method that can create plugin property files

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjernst can you point me to the helper class/method?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PluginTestUtil.writeProperties

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other methods in InstallPluginCommandTests that could potentially be useful if we wanted to share them.

"version=" + Version.CURRENT.toString(), "name=fake_plugin", "name=fake_plugin",
"elasticsearch.version=" + Version.CURRENT.toString(), "java.version=1.8", "classname=org.fake");
Files.write(configFile, config, Charset.forName("UTF-8"));
String[] params = { "-v" };
MockTerminal terminal = listPlugins(env, params);
String output = terminal.getOutput();
assertTrue(output, output.contains("Plugin information"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need stronger assertions here too.

}

public void testPluginWithVerboseMultiplePlugins() throws Exception {
Environment env = createEnv();
Files.createDirectory(env.pluginsFile().resolve("fake1"));
Files.createDirectory(env.pluginsFile().resolve("fake2"));
Files.createDirectory(env.pluginsFile().resolve("fake3"));
Path configFile = Files.createFile(env.pluginsFile().resolve("fake1").resolve(PluginInfo.ES_PLUGIN_PROPERTIES));
Path configFile2 = Files.createFile(env.pluginsFile().resolve("fake2").resolve(PluginInfo.ES_PLUGIN_PROPERTIES));
Path configFile3 = Files.createFile(env.pluginsFile().resolve("fake3").resolve(PluginInfo.ES_PLUGIN_PROPERTIES));
List<String> config = Arrays.asList("description=fake plugin",
"version=" + Version.CURRENT.toString(), "name=fake_plugin", "name=fake_plugin",
"elasticsearch.version=" + Version.CURRENT.toString(), "java.version=1.8", "classname=org.fake");
Files.write(configFile, config, Charset.forName("UTF-8"));
Files.write(configFile2, config, Charset.forName("UTF-8"));
Files.write(configFile3, config, Charset.forName("UTF-8"));
String[] params = { "-v" };
MockTerminal terminal = listPlugins(env, params);
String output = terminal.getOutput();
assertTrue(output, output.split("Plugin information").length == 4);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertions here need to stronger than this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like assertTrue style assertions as the message when the assertion fails is useless. The matchers give much more useful information when an assertion fails. For example:

assertThat(output.split("Plugin information"), arrayWithSize(4));

That comment applies to all uses of assertTrue and assertFalse here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with this comment. I think using assertTrue and assertFalse is just fine. The message is not useless when you add eg the output like is done here. No matter what the message says, someone looking into a test failure needs to look at the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe i can change all the messages from the class?

I was actually trying to follow the convention on the existing tests in this class. Although it seems that this class was using old/wrong formats?

Copy link
Member

@jasontedor jasontedor May 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message is not useless when you add eg the output like is done here.

This I disagree with. The output is just the contents of output, that doesn't say what failed. You get that for free using the matchers and I find it valuable.

No matter what the message says, someone looking into a test failure needs to look at the test.

This I agree with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change some of the assertions to use assertEquals. The message is actually much better than assert True.

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a newline here between the brace closing the method and the brace closing the class.

}