-
Notifications
You must be signed in to change notification settings - Fork 25k
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.
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
Changes from 4 commits
397c9f1
8307a05
f94a749
fb2c1b7
234bc68
ffe41ec
206d2f9
9f87f9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,15 +20,21 @@ | |
package org.elasticsearch.plugins; | ||
|
||
import java.io.IOException; | ||
import java.nio.charset.Charset; | ||
import java.nio.file.Files; | ||
import java.nio.file.NoSuchFileException; | ||
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.cli.UserError; | ||
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 { | ||
|
@@ -43,13 +49,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()); | ||
|
@@ -66,18 +76,150 @@ public void testNoPlugins() throws Exception { | |
|
||
public void testOnePlugin() throws Exception { | ||
Environment env = createEnv(); | ||
Files.createDirectory(env.pluginsFile().resolve("fake")); | ||
PluginTestUtil.writeProperties(env.pluginsFile().resolve("fake"), | ||
"description", "fake desc", | ||
"name", "fake_plugin", | ||
"version", "1.0", | ||
"elasticsearch.version", Version.CURRENT.toString(), | ||
"java.version", System.getProperty("java.specification.version"), | ||
"classname", "org.fake"); | ||
|
||
MockTerminal terminal = listPlugins(env); | ||
assertTrue(terminal.getOutput(), terminal.getOutput().contains("fake")); | ||
} | ||
|
||
public void testTwoPlugins() throws Exception { | ||
Environment env = createEnv(); | ||
Files.createDirectory(env.pluginsFile().resolve("fake1")); | ||
Files.createDirectory(env.pluginsFile().resolve("fake2")); | ||
PluginTestUtil.writeProperties(env.pluginsFile().resolve("fake1"), | ||
"description", "fake desc", | ||
"name", "fake_plugin", | ||
"version", "1.0", | ||
"elasticsearch.version", Version.CURRENT.toString(), | ||
"java.version", System.getProperty("java.specification.version"), | ||
"classname", "org.fake"); | ||
PluginTestUtil.writeProperties(env.pluginsFile().resolve("fake2"), | ||
"description", "fake desc", | ||
"name", "fake2_plugin", | ||
"version", "1.0", | ||
"elasticsearch.version", Version.CURRENT.toString(), | ||
"java.version", System.getProperty("java.specification.version"), | ||
"classname", "org.fake"); | ||
MockTerminal terminal = listPlugins(env); | ||
String output = terminal.getOutput(); | ||
assertTrue(output, output.contains("fake1")); | ||
assertTrue(output, output.contains("fake2")); | ||
} | ||
|
||
public void testPluginWithVerbose() throws Exception { | ||
Environment env = createEnv(); | ||
PluginTestUtil.writeProperties(env.pluginsFile().resolve("fake1"), | ||
"description", "fake desc", | ||
"name", "fake_plugin", | ||
"version", "1.0", | ||
"elasticsearch.version", Version.CURRENT.toString(), | ||
"java.version", System.getProperty("java.specification.version"), | ||
"classname", "org.fake"); | ||
String[] params = { "-v" }; | ||
MockTerminal terminal = listPlugins(env, params); | ||
String output = terminal.getOutput(); | ||
assertTrue(output, output.contains("Plugin information")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will need stronger assertions here too. |
||
assertTrue(output, output.contains("Classname: org.fake")); | ||
assertTrue(output, output.contains("fake_plugin")); | ||
assertTrue(output, output.contains("1.0")); | ||
assertTrue(output, output.contains("fake desc")); | ||
} | ||
|
||
public void testPluginWithVerboseMultiplePlugins() throws Exception { | ||
Environment env = createEnv(); | ||
|
||
PluginTestUtil.writeProperties(env.pluginsFile().resolve("fake1"), | ||
"description", "fake desc", | ||
"name", "fake_plugin", | ||
"version", "1.0", | ||
"elasticsearch.version", Version.CURRENT.toString(), | ||
"java.version", System.getProperty("java.specification.version"), | ||
"classname", "org.fake"); | ||
|
||
PluginTestUtil.writeProperties(env.pluginsFile().resolve("fake2"), | ||
"description", "fake desc", | ||
"name", "fake2_plugin", | ||
"version", "1.0", | ||
"elasticsearch.version", Version.CURRENT.toString(), | ||
"java.version", System.getProperty("java.specification.version"), | ||
"classname", "org.fake2"); | ||
|
||
PluginTestUtil.writeProperties(env.pluginsFile().resolve("fake3"), | ||
"description", "fake desc", | ||
"name", "fake3_plugin", | ||
"version", "1.0", | ||
"elasticsearch.version", Version.CURRENT.toString(), | ||
"java.version", System.getProperty("java.specification.version"), | ||
"classname", "org.fake3"); | ||
|
||
String[] params = { "-v" }; | ||
MockTerminal terminal = listPlugins(env, params); | ||
String output = terminal.getOutput(); | ||
|
||
//Should be 3 plugins, so if i split by plugin informations i should get 4 strings | ||
assertTrue(output, output.split("Plugin information").length == 4); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The assertions here need to stronger than this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like assertThat(output.split("Plugin information"), arrayWithSize(4)); That comment applies to all uses of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This I disagree with. The output is just the contents of
This I agree with. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
assertTrue(output, output.contains("Classname: org.fake")); | ||
assertTrue(output, output.contains("Classname: org.fake2")); | ||
assertTrue(output, output.contains("Classname: org.fake3")); | ||
assertTrue(output, output.contains("fake_plugin")); | ||
assertTrue(output, output.contains("fake2_plugin")); | ||
assertTrue(output, output.contains("fake3_plugin")); | ||
assertTrue(output, output.contains("1.0")); | ||
assertTrue(output, output.contains("fake desc")); | ||
} | ||
|
||
public void testPluginWithoutVerboseMultiplePlugins() throws Exception { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this test adds anything over the existing There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it! Good idea. |
||
Environment env = createEnv(); | ||
|
||
PluginTestUtil.writeProperties(env.pluginsFile().resolve("fake1"), | ||
"description", "fake desc", | ||
"name", "fake_plugin", | ||
"version", "1.0", | ||
"elasticsearch.version", Version.CURRENT.toString(), | ||
"java.version", System.getProperty("java.specification.version"), | ||
"classname", "org.fake"); | ||
|
||
PluginTestUtil.writeProperties(env.pluginsFile().resolve("fake2"), | ||
"description", "fake desc", | ||
"name", "fake2_plugin", | ||
"version", "1.0", | ||
"elasticsearch.version", Version.CURRENT.toString(), | ||
"java.version", System.getProperty("java.specification.version"), | ||
"classname", "org.fake2"); | ||
|
||
PluginTestUtil.writeProperties(env.pluginsFile().resolve("fake3"), | ||
"description", "fake desc", | ||
"name", "fake3_plugin", | ||
"version", "1.0", | ||
"elasticsearch.version", Version.CURRENT.toString(), | ||
"java.version", System.getProperty("java.specification.version"), | ||
"classname", "org.fake3"); | ||
|
||
MockTerminal terminal = listPlugins(env, new String[0]); | ||
String output = terminal.getOutput(); | ||
assertFalse(output, output.contains("Plugin information")); | ||
} | ||
|
||
public void testPluginWithoutDescriptionFile() throws Exception{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
Environment env = createEnv(); | ||
Files.createDirectories(env.pluginsFile().resolve("fake1")); | ||
NoSuchFileException e = expectThrows(NoSuchFileException.class, () -> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we convert this off of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there is a bug here. What is |
||
} | ||
|
||
public void testPluginWithWrongDescriptionFile() throws Exception{ | ||
Environment env = createEnv(); | ||
PluginTestUtil.writeProperties(env.pluginsFile().resolve("fake1"), | ||
"description", "fake desc"); | ||
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we convert this off of |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.