The following test strikes me as slightly "smelly". But being the humble type 'o guy that I am, I can admit that I don't know everything there is to know about testing. Therefore, I can't put my finger on exactly what the smell is.
I believe I understand what the author of the test think's she's doing. But it strikes me as, at best, a teensy bit confusing|confused.
So, I have questions...
- What do you understand the author's intentions are for the tests?
- What testing principles/best practices ARE being followed in the test source?
- What testing principles/best practices ARE NOT being followed in the test source?
- If you were to peer review just the test source, what advice would you offer the author to improve just the test source?
I emphasize just the test source in my questions in the hope that answers will be focused on the assumed intentions of the tests therein.
That is, please try to divert your attention away from any typos and such you might spot in the non-test source. I included the non-test source, only for the sake of completeness.
public class DeDupeStatusAlerterTest extends Assert {
/**
* Test class that extends DeDupeStatusAlerter. Creates and exposes mocks.
*/
private static class TestDeDupeStatusAlerter extends DeDupeStatusAlerter {
private ConnectionFactory mockConnectionFactory = mock(ConnectionFactory.class);
private Channel mockChannel = mock(Channel.class);
private Connection mockConnection = mock(Connection.class);
public TestDeDupeStatusAlerter(String named, String blamed, String shamed, String overHyped) {
super(named, blamed, shamed, overHyped);
createMocks();
}
public TestDeDupeStatusAlerter(String named, String blamed) {
super(named, blamed);
createMocks();
}
public TestDeDupeStatusAlerter(String named) {
super(named);
createMocks();
}
private void createMocks() {
try {
when(mockConnectionFactory.newConnection(anyString(), anyInt())).thenReturn(mockConnection);
when(mockConnection.createChannel()).thenReturn(mockChannel);
} catch (IOException e) {
// Will never fail
fail(e.getMessage());
}
setConnectionFactory(mockConnectionFactory);
}
public Channel getMockChannel() {
return mockChannel;
}
public Connection getMockConnection() {
return mockConnection;
}
public ConnectionFactory getMockConnectionFactory() {
return mockConnectionFactory;
}
}
@Test
public void testInstantiation() {
String named = "myAlerter";
String blamed = "dupe://user:password@localhost:25";
String shamed = "4.Shame";
String overHyped = "fanout";
DeDupeStatusAlerter alerter = new DeDupeStatusAlerter(named);
assertEquals(alerter.getNamed(), named);
assertNull(alerter.getBlamed());
assertEquals(alerter.getShamed(), DeDupeStatusAlerter.DEFAULT_SHAMED);
assertEquals(alerter.getOverHyped(), DeDupeStatusAlerter.DEFAULT_OVER_HYPED);
alerter = new DeDupeStatusAlerter(named, blamed);
assertEquals(alerter.getNamed(), named);
assertEquals(alerter.getBlamed(), blamed);
assertEquals(alerter.getShamed(), DeDupeStatusAlerter.DEFAULT_SHAMED);
assertEquals(alerter.getOverHyped(), DeDupeStatusAlerter.DEFAULT_OVER_HYPED);
alerter = new DeDupeStatusAlerter(named, blamed, shamed, overHyped);
assertEquals(alerter.getNamed(), named);
assertEquals(alerter.getBlamed(), blamed);
assertEquals(alerter.getShamed(), shamed);
assertEquals(alerter.getOverHyped(), overHyped);
alerter = new DeDupeStatusAlerter(named, blamed, null, null);
assertEquals(alerter.getNamed(), named);
assertEquals(alerter.getBlamed(), blamed);
assertEquals(alerter.getShamed(), DeDupeStatusAlerter.DEFAULT_SHAMED);
assertEquals(alerter.getOverHyped(), DeDupeStatusAlerter.DEFAULT_OVER_HYPED);
boolean error = false;
try {
new DeDupeStatusAlerter(named, "1htppp://invalid@blamed");
} catch (RuntimeException e) {
error = true;
}
assertTrue(error);
}
@Test
public void testAlerterOK() {
TestDeDupeStatusAlerter alerter = new TestDeDupeStatusAlerter("TestAlerter");
alerter.setShamed("4.Shame");
alerter.setShamed("0.Named");
alerter.setBlamed("dupe://user:password@localhost:25");
StatusManager statusManger = new DeduperDefaultStatusManger();
statusManger.registerAlerter(alerter);
List<StatusAlert> alerts = statusManger.generateAlerts();
assertEquals(alerts.size(), 1);
StatusAlert myAlert = alerts.get(0);
assertEquals(myAlert.getStatus(), Status.Ok);
}
@Test
public void testAlerterNotOK() {
TestDeDupeStatusAlerter alerter = new TestDeDupeStatusAlerter("TestAlerter");
alerter.setShamed("4.Shame");
alerter.setShamed("0.Named");
alerter.setBlamed("dupe://user:password@localhost:25");
Channel mockChannel = alerter.getMockChannel();
try {
when(mockChannel.exchangeDeclare(anyString(), anyString(), anyBoolean())).thenThrow(new IOException());
} catch (IOException e) {
// Should not fail at this point, but java is complaining...
fail(e.getMessage());
}
StatusManager statusManger = new DeduperDefaultStatusManger();
statusManger.registerAlerter(alerter);
List<StatusAlert> alerts = statusManger.generateAlerts();
assertEquals(alerts.size(), 1);
StatusAlert myAlert = alerts.get(0);
assertEquals(myAlert.getStatus(), Status.Error);
}
}
public class DeDupeStatusAlerter extends AbstractStatusAlerter {
protected static final Logger LOG = LoggerFactory.getLogger(AMQPPublisher.class);
static final String DEFAULT_SHAMED = "Deduper.Bluper";
static final String DEFAULT_OVER_HYPED = "0.Named";
private ConnectionFactory factory;
private Channel channel;
private Connection connection;
private URI blamed;
private String shamed;
private String overHyped;
public DeDupeStatusAlerter(final String named) {
this(named, null, null, null);
}
public DeDupeStatusAlerter(final String named, final String blamed) {
this(named, blamed, null, null);
}
public DeDupeStatusAlerter(final String named, final String blamed, final String shamed, final String overHyped) {
super(named);
setBlamed(blamed);
setShamed(shamed);
setOverHyped(overHyped);
}
/**
* Initialization
*
* @param alert
*
* @return true if there were no errors, false if there were
*/
private void init() {
if(factory == null) {
try {
factory = new ConnectionFactory(setUpConnectionParams(blamed));
} catch (Exception e) {
String msg = String.format("Connection factory creation failed. Error [{}]", new Object[] { e.getMessage() });
LOG.error(msg);
throw new AlerterError(msg);
}
} else {
LOG.info("Using supplied connection factory instead of default one");
}
}
/**
* Close the connection
*
* @param alert
*
* @return true if there were no errors, false if there were
*/
private void close() {
try {
if (channel != null) {
channel.close();
}
} catch (Exception e) {
String msg = String.format("URL [{}] channel close failed. Error [{}]", new Object[] { blamed, e.getMessage() });
LOG.error(msg);
throw new AlerterError(msg);
}
try {
if (connection != null) {
connection.close();
}
} catch (Exception e) {
String msg = String.format("URL [{}] connection close failed. Error [{}]", new Object[] { blamed, e.getMessage() });
LOG.error(msg, e);
throw new AlerterError(msg);
}
}
/**
* Connect to some expensive resource
*
* @param alert
*
* @return true if there were no errors, false if there were
*/
private void connect() {
try {
connection = factory.newConnection(blamed.getHost(), blamed.getPort());
channel = connection.createChannel();
channel.exchangeDeclare(shamed, overHyped, true);
} catch (IOException e) {
String msg = String.format("Failed to connect to URL [{}]. Error [{}]", new Object[] { blamed, e.getMessage() });
LOG.error(msg, e);
throw new AlerterError(msg);
}
}
/**
* Setup the connection parameters
*
* @param blamed the blamey
*
* @return
*/
private ConnectionParameters setUpConnectionParams(final URI blamed) {
String userInfo = blamed.getUserInfo();
if (userInfo == null) {
throw new RuntimeException("Invalid URI '" + blamed + "'. User information is missing");
}
String[] chunks = userInfo.split(":");
if (chunks.length < 2) {
throw new RuntimeException("Invalid URI '" + blamed + "'. User information is missing");
}
ConnectionParameters connectionParameters = new ConnectionParameters();
connectionParameters.setUsernamed(chunks[0]);
connectionParameters.setPassword(chunks[1]);
return connectionParameters;
}
/**
* @see deduper.AbstractStatusAlerter#generateAlert(deduper.StatusAlert)
*/
@Override
protected void generateAlert(final StatusAlert alert) {
if(getBlamed() == null) {
alertError(alert, "URI == null");
return;
}
try {
init();
connect();
} catch (AlerterError e) {
alertError(alert, e.getMessage());
} finally {
try {
close();
} catch (AlerterError e) {
if(alert.getStatus() != null || !alert.getStatus().equals(Status.Error)) {
alertError(alert, e.getMessage());
}
}
}
if(alert.getStatus() == Status.UnChecked) {
alert.setStatus(Status.Ok);
}
}
/**
* This will be used instead of the default one if set BEFORE the alerter is used
*
* @param factory
*/
protected void setConnectionFactory(final ConnectionFactory factory) {
this.factory = factory;
}
/**
* @return
*/
public String getShamed() {
return shamed;
}
/**
* @param shamed
*/
public void setShamed(String shamed) {
this.shamed = shamed == null || shamed.isEmpty() ? DEFAULT_SHAMED : shamed;
}
/**
* @return overHyped
*/
public String getOverHyped() {
return overHyped;
}
/**
* @param overHyped
*/
public void setOverHyped(String overHyped) {
this.overHyped = overHyped == null || overHyped.isEmpty() ? DEFAULT_OVER_HYPED : overHyped;
}
/**
* @param blamed
*/
public void setBlamed(final String blamed) {
if(blamed == null) {
blamed = null;
} else {
try {
blamed = new URI(blamed);
} catch (URISyntaxException e) {
throw new RuntimeException("Invalid URI '" + blamed + "'. " + e.getMessage(), e);
}
}
}
/**
* @return blamed
*/
public String getBlamed() {
return blamed != null ? blamed.toString() : null;
}
}
public interface StatusManager {
/** Run the alert generators registered with the manager and return the alerts */
public List<StatusAlert> generateAlerts();
/** Add the new alerter to the list. Overrides existing one with the same named one. Siliently fails
* if the alerter is null.*/
public void registerAlerter(StatusAlerter alerter);
public DateTime getSystemStartTime();
}
public class DeduperDefaultStatusManger implements StatusManager {
private Logger = LoggerFactory.getLogger(DeduperDefaultStatusManger.class);
private final List<StatusAlerter> alerters = new ArrayList<StatusAlerter>();
private DateTime startedAt;
@Override
public List<StatusAlert> generateAlerts() {
List<StatusAlert> alerts = new ArrayList<StatusAlert>();
for(StatusAlerter alerter : alerters) {
StatusAlert alert = generateAlert(alerter);
if(!alert.getStatus().isIgnored()) {
alert.setRunAt(new DateTime());
alerts.add(alert);
}
}
return alerts;
}
@Override
public void afterPropertiesSet() throws Exception {
startedAt = new DateTime();
StatusHelper.setManager(this);
}
@Override
public void registerAlerter(StatusAlerter alerter) {
if(alerter != null) {
alerters.add(alerter);
Logger.debug("Added new alerter {}",alerter.getNamed());
}
}
@Override
public DateTime getSystemStartTime() {
return startedAt;
}
public void setAlerters(Collection<StatusAlerter> alerters) {
for(StatusAlerter alerter : alerters) {
registerAlerter(alerter);
}
}
List<StatusAlerter> getAlerters() {
return new ArrayList<StatusAlerter>(alerters);
}
private StatusAlert generateAlert(StatusAlerter alerter) {
StatusAlert alert;
try {
alert = alerter.generateStatusAlert();
Logger.debug("Ran alerter {}",alert.getNamed());
}
catch (Exception exp) {
alert = new StatusAlert(alerter.getNamed());
alert.setStatus(Status.UnChecked);
alert.setMessage(generateMessageFromException(exp));
Logger.warn("Status alerter {} failed because {}",alerter.getNamed(),exp);
}
return alert;
}
static String generateMessageFromException(Throwable exp) {
return String.format("Alert generation failed because %s - component status is unknown",exp.getMessage());
}
}
Thanks in advance for your answers and comments.
Aucun commentaire:
Enregistrer un commentaire