-
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
CLI: Close subcommands in MultiCommand #28954
Changes from 14 commits
39770da
be11085
bf6a3eb
33d78c4
63c3656
52a71d9
ee04957
103d175
050a9f1
b2ece8a
99dd6ce
4b42afd
6eb66e7
91d785e
862090e
f098095
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 |
---|---|---|
|
@@ -22,22 +22,57 @@ | |
import joptsimple.OptionSet; | ||
import org.junit.Before; | ||
|
||
import java.io.IOException; | ||
import java.util.concurrent.atomic.AtomicBoolean; | ||
|
||
public class MultiCommandTests extends CommandTestCase { | ||
|
||
static class DummyMultiCommand extends MultiCommand { | ||
|
||
final AtomicBoolean closed = new AtomicBoolean(); | ||
|
||
DummyMultiCommand() { | ||
super("A dummy multi command", () -> {}); | ||
super("A dummy multi command", () -> { | ||
}); | ||
} | ||
|
||
@Override | ||
public void close() throws IOException { | ||
super.close(); | ||
if (this.closed.compareAndSet(false, true) == false) { | ||
throw new IOException("DummyMultiCommand already closed"); | ||
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. IMO this is not really an issue with IO so |
||
} | ||
} | ||
} | ||
|
||
static class DummySubCommand extends Command { | ||
final boolean throwsExceptionOnClose; | ||
final AtomicBoolean closeCalled = new AtomicBoolean(); | ||
|
||
DummySubCommand() { | ||
super("A dummy subcommand", () -> {}); | ||
this(false); | ||
} | ||
|
||
DummySubCommand(final boolean throwsExceptionOnClose) { | ||
super("A dummy subcommand", () -> { | ||
}); | ||
this.throwsExceptionOnClose = throwsExceptionOnClose; | ||
} | ||
|
||
@Override | ||
protected void execute(Terminal terminal, OptionSet options) throws Exception { | ||
terminal.println("Arguments: " + options.nonOptionArguments().toString()); | ||
} | ||
|
||
@Override | ||
public void close() throws IOException { | ||
if (this.closeCalled.compareAndSet(false, true) == false) { | ||
throw new IOException("DummySubCommand already closed"); | ||
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. same thing here, how about using |
||
} | ||
if (throwsExceptionOnClose) { | ||
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. If you have this condition+throw separate from setting closed (maybe rename closed to closeCalled) then you don't need the difficult to read xor logic in the last test. |
||
throw new IOException("Error occurred while closing DummySubCommand"); | ||
} | ||
} | ||
} | ||
|
||
DummyMultiCommand multiCommand; | ||
|
@@ -102,4 +137,39 @@ public void testSubcommandArguments() throws Exception { | |
assertFalse(output, output.contains("command1")); | ||
assertTrue(output, output.contains("Arguments: [foo, bar]")); | ||
} | ||
|
||
public void testClose() throws Exception { | ||
DummySubCommand subCommand1 = new DummySubCommand(); | ||
DummySubCommand subCommand2 = new DummySubCommand(); | ||
multiCommand.subcommands.put("command1", subCommand1); | ||
multiCommand.subcommands.put("command2", subCommand2); | ||
multiCommand.close(); | ||
assertTrue("MultiCommand was not closed when close method is invoked", multiCommand.closed.get()); | ||
assertTrue("SubCommand1 was not closed when close method is invoked", subCommand1.closeCalled.get()); | ||
assertTrue("SubCommand2 was not closed when close method is invoked", subCommand2.closeCalled.get()); | ||
} | ||
|
||
public void testCloseWhenSubCommandCloseThrowsException() throws Exception { | ||
final boolean command1Throws = randomBoolean(); | ||
final boolean command2Throws = randomBoolean(); | ||
final DummySubCommand subCommand1 = new DummySubCommand(command1Throws); | ||
final DummySubCommand subCommand2 = new DummySubCommand(command2Throws); | ||
multiCommand.subcommands.put("command1", subCommand1); | ||
multiCommand.subcommands.put("command2", subCommand2); | ||
if (command1Throws || command2Throws) { | ||
// verify exception is thrown, as well as other non failed sub-commands closed | ||
// properly. | ||
IOException ioe = expectThrows(IOException.class, multiCommand::close); | ||
assertEquals("Error occurred while closing DummySubCommand", ioe.getMessage()); | ||
if (command1Throws && command2Throws) { | ||
assertEquals(1, ioe.getSuppressed().length); | ||
assertTrue("Missing suppressed exceptions", ioe.getSuppressed()[0] instanceof IOException); | ||
assertEquals("Error occurred while closing DummySubCommand", ioe.getSuppressed()[0].getMessage()); | ||
} | ||
} else { | ||
multiCommand.close(); | ||
} | ||
assertTrue("SubCommand1 was not closed when close method is invoked", subCommand1.closeCalled.get()); | ||
assertTrue("SubCommand2 was not closed when close method is invoked", subCommand2.closeCalled.get()); | ||
} | ||
} |
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 is a personal preference but given that we're adding to this class, I find it hard to read without empty lines between methods/constructor/member fields. Can you please add them?