Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

METRON-1299 In MetronError tests, don't test for HostName if getHostName wouldn't work #924

Closed
wants to merge 2 commits into from

Conversation

ottobackwards
Copy link
Contributor

MetronError ignores exceptions from InetAddress.getLocalHost().getHostName() and leaves the field unset.

The unit test however assumes it would be set, and someone has logged a jira on this, since it makes the build fail.

Changed the test so that it only verifies the hostName if it would have worked.

Testing

  • Code review
  • Tests Pass

no non-test changes in pr

 private void addHostname(JSONObject errorMessage) {
    try {
      errorMessage.put(ErrorFields.HOSTNAME.getName(), InetAddress.getLocalHost().getHostName());
    } catch (UnknownHostException ex) {
      // Leave the hostname field off if it cannot be found
    }
  }

For all changes:

  • Is there a JIRA ticket associated with this PR? If not one needs to be created at Metron Jira.
  • Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
  • Has your PR been rebased against the latest commit within the target branch (typically master)?

assertTrue(((String) errorJSON.get(Constants.ErrorFields.HOSTNAME.getName())).length() > 0);

try {
String hostName = InetAddress.getLocalHost().getHostName();
Copy link
Member

Choose a reason for hiding this comment

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

So, in the instance where this happens, is hostName is null or is it that there's an Exception thrown? If it's null, can't we do a:

if(hostName == null) {
  LOG.warn("Unable to resolve local hostname, skipping validation."
}
else {
  assertTrue(((String) errorJSON.get(Constants.ErrorFields.HOSTNAME.getName())).length() > 0);
  assertEquals(hostName, (String) errorJSON.get(Constants.ErrorFields.HOSTNAME.getName()));
}

I ask because I worry about extraneous other exceptions cropping up and us ignoring them. It's a very small nit, let me know what you think. :)

@merrimanr
Copy link
Contributor

+1 pending @cestella's approval. Thanks @ottobackwards.

@justinleet
Copy link
Contributor

@cestella Bump

@cestella
Copy link
Member

cestella commented Mar 5, 2018

+1, sorry!

@asfgit asfgit closed this in 85d1247 Mar 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants