Fixes issue #84 - Support multiple config files #86
Conversation
Codecov Report
@@ Coverage Diff @@
## master #86 +/- ##
==========================================
- Coverage 98.98% 98.29% -0.69%
==========================================
Files 35 36 +1
Lines 1082 1117 +35
Branches 178 182 +4
==========================================
+ Hits 1071 1098 +27
- Misses 4 10 +6
- Partials 7 9 +2
Continue to review full report at Codecov.
|
| ); | ||
| } | ||
|
|
||
| List<Path> files = findAllYaml(configDir); |
There was a problem hiding this comment.
What is the need to actually write into a merged temp file? Why not to just merge loaded sub-files in memory? Also, did you test the merge functionality when --watch command line argument is provided - will the changes to one of the sub files cause a reload?
| long count = Files.lines(merged.toPath()) | ||
| .filter(line -> line.trim().equals(str)) | ||
| .count(); | ||
| assertThat(count).isEqualTo(3L); |
There was a problem hiding this comment.
Can you please avoid from asserting for something in the loop?
|
|
||
| File merged = YAMLMerger.merge(resources.toFile()); | ||
| assertThat(merged).isNotNull(); | ||
| assertThat(merged.exists() && merged.isFile() && merged.canRead()).isTrue(); |
There was a problem hiding this comment.
Can you please break the boolean && boolean && boolean up to three assertThat statements? If something fails, it is less clear what exactly failed
| */ | ||
| public class YAMLMergerTest { | ||
| @Test | ||
| public void testMerge() throws IOException, URISyntaxException { |
There was a problem hiding this comment.
This test is testing too much. Can you please break it up for several atomic tests that tests different parts of the behavior?
| */ | ||
| public class YAMLMergerTest { | ||
| @Test | ||
| public void testMerge() throws IOException, URISyntaxException { |
There was a problem hiding this comment.
It is OK for tests to throws Exception if you are not testing for specific exception
| public final class YAMLMerger { | ||
| private static final Logger LOGGER = LoggerFactory.getLogger(YAMLMerger.class); | ||
| private static final PathMatcher YAML_MATCHER = FileSystems.getDefault() | ||
| .getPathMatcher("glob:**.{yaml,yml}"); |
There was a problem hiding this comment.
There are no unit tests for "glob:**.{yaml,yml}"
|
Sorry, I've exhausted the time I estimated for working on this CR. You can:
|
Fixes issue #84 - Support multiple config files