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 5 commits
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
2 changes: 2 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,8 @@ 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 = PluginInfo.readFromProperties(env.pluginsFile().resolve(plugin.toAbsolutePath()));
terminal.println(Terminal.Verbosity.VERBOSE, info.toString());
}
}
}
Expand Down
87 changes: 79 additions & 8 deletions 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 @@ -19,16 +19,21 @@

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.file.Files;
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.stream.Collectors;

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,20 +48,38 @@ 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;
}

static String buildMultiline(String... args){
return Arrays.asList(args).stream().collect(Collectors.joining("\n", "", "\n"));
}

static void buildFakePlugin(Environment env, String description, String name, String classname) throws IOException {
PluginTestUtil.writeProperties(env.pluginsFile().resolve(name),
"description", description,
"name", name,
"version", "1.0",
"elasticsearch.version", Version.CURRENT.toString(),
"java.version", System.getProperty("java.specification.version"),
"classname", classname);
}


public void testPluginsDirMissing() throws Exception {
Environment env = createEnv();
Files.delete(env.pluginsFile());
IOException e = expectThrows(IOException.class, () -> {
listPlugins(env);
});
assertTrue(e.getMessage(), e.getMessage().contains("Plugins directory missing"));
assertEquals(e.getMessage(), "Plugins directory missing: " + env.pluginsFile());
}

public void testNoPlugins() throws Exception {
Expand All @@ -66,18 +89,66 @@ public void testNoPlugins() throws Exception {

public void testOnePlugin() throws Exception {
Environment env = createEnv();
Files.createDirectory(env.pluginsFile().resolve("fake"));
buildFakePlugin(env, "fake desc", "fake", "org.fake");
MockTerminal terminal = listPlugins(env);
assertTrue(terminal.getOutput(), terminal.getOutput().contains("fake"));
assertEquals(terminal.getOutput(), buildMultiline("fake"));
}

public void testTwoPlugins() throws Exception {
Environment env = createEnv();
Files.createDirectory(env.pluginsFile().resolve("fake1"));
Files.createDirectory(env.pluginsFile().resolve("fake2"));
buildFakePlugin(env, "fake desc", "fake1", "org.fake");
buildFakePlugin(env, "fake desc 2", "fake2", "org.fake");
MockTerminal terminal = listPlugins(env);
assertEquals(terminal.getOutput(), buildMultiline("fake1", "fake2"));
}

public void testPluginWithVerbose() throws Exception {
Environment env = createEnv();
buildFakePlugin(env, "fake desc", "fake_plugin", "org.fake");
String[] params = { "-v" };
MockTerminal terminal = listPlugins(env, params);
assertEquals(terminal.getOutput(), buildMultiline("Plugins directory: " + env.pluginsFile(), "fake_plugin",
"- Plugin information:", "Name: fake_plugin", "Description: fake desc", "Version: 1.0", " * Classname: org.fake"));
}

public void testPluginWithVerboseMultiplePlugins() throws Exception {
Environment env = createEnv();
buildFakePlugin(env, "fake desc 1", "fake_plugin1", "org.fake");
buildFakePlugin(env, "fake desc 2", "fake_plugin2", "org.fake2");
String[] params = { "-v" };
MockTerminal terminal = listPlugins(env, params);
assertEquals(terminal.getOutput(), buildMultiline("Plugins directory: " + env.pluginsFile(),
"fake_plugin1", "- Plugin information:", "Name: fake_plugin1", "Description: fake desc 1", "Version: 1.0",
" * Classname: org.fake", "fake_plugin2", "- Plugin information:", "Name: fake_plugin2",
"Description: fake desc 2", "Version: 1.0", " * Classname: org.fake2"));
}

public void testPluginWithoutVerboseMultiplePlugins() throws Exception {
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.

I don't think this test adds anything over the existing testOnePlugin and testTwoPlugins tests (although the assertions in those tests should probably be tightened to specify the exact output). I think that that should be done as part of this pull request since this pull request is modifying the output depending on a command-line flag and thus we need those tests strengthened to ensure that when verbose is not set that the output there was not modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we assert both things in the other tests, matching the verbose and non-verbose output?

Copy link
Member

Choose a reason for hiding this comment

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

I think that the both kinds of tests (verbose and non-verbose) should specify exactly what the output is. Note that as written the non-verbose tests would pass even if the production code was changed to always output the verbose output. So, the output would be wrong but the test would not fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! Good idea.

Environment env = createEnv();
buildFakePlugin(env, "fake desc 1", "fake_plugin1", "org.fake");
buildFakePlugin(env, "fake desc 2", "fake_plugin2", "org.fake2");
MockTerminal terminal = listPlugins(env, new String[0]);
String output = terminal.getOutput();
assertTrue(output, output.contains("fake1"));
assertTrue(output, output.contains("fake2"));
assertEquals(output, buildMultiline("fake_plugin1", "fake_plugin2"));
}

public void testPluginWithoutDescriptionFile() throws Exception{
Copy link
Member

Choose a reason for hiding this comment

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

Description -> Descriptor

Environment env = createEnv();
Files.createDirectories(env.pluginsFile().resolve("fake1"));
NoSuchFileException e = expectThrows(NoSuchFileException.class, () -> {
Copy link
Member

Choose a reason for hiding this comment

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

This lambda does not need to be a statement block.

listPlugins(env);
});
assertTrue(e.getMessage(), e.getMessage().contains("fake1\\plugin-descriptor.properties"));
Copy link
Member

@jasontedor jasontedor May 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Can we convert this off of assertTrue, the error message is useless?

Copy link
Member

Choose a reason for hiding this comment

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

I think there is a bug here. What is \\? I guess Windows? You need to take caution for different filesystems.

}

public void testPluginWithWrongDescriptionFile() throws Exception{
Environment env = createEnv();
PluginTestUtil.writeProperties(env.pluginsFile().resolve("fake1"),
"description", "fake desc");
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> {
Copy link
Member

Choose a reason for hiding this comment

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

This lambda does not need to be a statement block.

listPlugins(env);
});
assertTrue(e.getMessage(), e.getMessage().contains("Property [name] is missing in"));
Copy link
Member

Choose a reason for hiding this comment

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

Can we convert this off of assertTrue, the error message is useless?

}
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.


}