MINIFI-290 Refactor Tests

[kevdoran]

- Correct doxygen function comment in YamlConfiguration
- Correct a log message in YamlConfiguration
- Tweak ubuntu Dockerfile to fix docker build target
- Add --output-on-failure flag to Travis config

Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

• Is there a JIRA ticket associated with this PR? Is it referenced
  in the commit message?

• Does your PR title start with MINIFI-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)?

• Is your initial contribution a single, squashed commit?

For code changes:

• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
• If applicable, have you updated the LICENSE file?
• If applicable, have you updated the NOTICE file?

For documentation related changes:

• Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

[phrocker]

LGTM, but may want to rename YamlConfiguration as there is a typo in it.

[apiri]

reviewing

[apiri]

Hey @kevdoran. This is certainly a lot cleaner and a much welcomed improvement. Might you be able to rebase on the changes that went in recently for the updates to how configure is handled and take care of renaming YamlConfiguration while we are messing with it in this commit?

Thanks!

[kevdoran]

Thanks for the feedback @apiri. Will do

[kevdoran]

@apiri and @phrocker - requested changes have been made. let me know if there is anything else here

[apiri]

@kevdoran Thanks for rebasing/adjusting. Will get this merged.

[apiri]

Saw that the Travis build for Linux failed. Doing some additional builds/runs to make sure we're clear first.

[kevdoran]

I'll run some more linux tests as well. Should we look into configuring the CTests to dump verbose output on failures so we could get more information out of Travis?

[kevdoran]

@apiri and @phrocker:

Here is the verbose test

-------------------------------------------------------------------------------
Test YAML Config Processing
  loading YAML without optional component IDs works
-------------------------------------------------------------------------------
/home/travis/build/kevdoran/nifi-minifi-cpp/libminifi/test/unit/YamlConfigurationTests.cpp:25
...............................................................................

/home/travis/build/kevdoran/nifi-minifi-cpp/libminifi/test/unit/YamlConfigurationTests.cpp:34: FAILED:
due to a fatal error condition:
  SIGSEGV - Segmentation violation signal

===============================================================================
test cases: 11 | 10 passed | 1 failed
assertions: 48 | 47 passed | 1 failed

1/6 Test #1: LibMinifiTests ...................***Failed    0.05 sec

It looks like this is failing for the same reason identified in MINIFI-304, which is resolved in @phrocker's pull request #96 [1]. Is that ready to merge?

[1] #96

[apiri]

Hey @kevdoran,

#96 is now merged. Would you be able to rebase and see if that solves the problem you were encountering before?

[kevdoran]

Will do.

[kevdoran]

@apiri @phrocker - update on this:

The approach of setting a hardcoded working directory when adding tests to ctest in the CMakeLists.txt file proved to be too brittle. If you invoke the test executable directly, it fails. After some exploration, I could not find a way to pass the resource location to the Catch unit tests in a manner that I was satisfied with. Two alternatives I explored included passing the location as an environment variable and passing it as a command line argument. The environment variable approach was a no-go as it it would require us to setup the "test" target in CMake using something other than enable_testing() and instead creating our own target, which seemed undesirable at this time. The argument approach would require us to define our own main for Catch unit tests, rather than using the #define CATCH_CONFIG_MAIN, and because #define CATCH_CONFIG_MAIN does more than just inline a main function, I didn't want to do that either in case assumptions made today break with other versions of Catch.hpp in the future.

The benefits here seemed minor, and could not justify the extra complexity introduced by the workarounds I explored (IMO), so I reverted the yaml config to being hardcoded inline strings. For now, I recommend closing MINIFI-290 as "won't do" and merge this PR which has a few other changes that I cam across while investigating this. Alternatively, we could move MINIFI-290 back to "open" if we want to revisit this in the future... but I'm sure if the need arises we would reopen it or make a new ticket.

[apiri]

@kevdoran thanks for all the thought process and rationale. totally seems fair. will review the updates. thanks!

[apiri]

@kevdoran Looks good overall just had one question on the block with the resources path which I believe is no longer needed. Let me know if that is the case and I can remove on merge. Thanks!

[apiri]

Is this a relic of the original approach? Don't believe it's currently being used any more.

[kevdoran]

Good catch! Will push a commit removing that to my branch

[kevdoran]

Updated

[apiri]

@kevdoran cool, thanks will get merged!