Add unit tests to verify yarn launcher config construction #1460
Conversation
| expectedMap.put(DriverMemory.class.getSimpleName(), "100"); | ||
|
|
||
| Config.Builder builder = new Config.Builder(); | ||
| for (String s : testConfigMap.keySet()) { |
There was a problem hiding this comment.
please rename 's' to something like configKey or something descriprive
|
Two general observations:
|
|
I think I can address the second observation. Regarding observation 1, most of the configurations provided to REEF are heron configs and are not optional. For e.g. there is no default value of a topology name. These are provided to launcher by heron config loader which is independently unit tested. I am not sure if this kind of test is applicable here? |
|
Updated the unit test. It will fail if unexpected config is found. |
| reefSpecificConfigs.remove(reefConfigNode.getName()); | ||
| } | ||
| } | ||
| Assert.assertEquals(0, expectedMap.size()); |
There was a problem hiding this comment.
If either of these assertions fails, the reason won't be clear to the reader of the logs. Would you please add a string reason for the failure (e.g., an unexpected config found) and output the map/set.
| setConfigs(Keys.corePackageUri(), new File(".").getName(), HeronCorePackageName.class); | ||
|
|
||
| Set<String> reefSpecificConfigs = new HashSet<>(); | ||
| reefSpecificConfigs.add("JobControlHandler"); |
There was a problem hiding this comment.
Can these key values be static strings referenced from somewhere?
| reefSpecificConfigs.add("DriverIdentifier"); | ||
|
|
||
| Config.Builder builder = new Config.Builder(); | ||
| for (String s : testConfigMap.keySet()) { |
There was a problem hiding this comment.
please use a more descriptive field name than s.
|
|
||
| for (NamedParameterNode<?> reefConfigNode : constructedConfig.getNamedParameters()) { | ||
| String value = expectedMap.get(reefConfigNode.getName()); | ||
| if (value != null) { |
There was a problem hiding this comment.
There are enough Map, Set, Config, Builder objects all referring to different expected/found/reef values that reading the test can get tricky to a reader without details of the config code implementation. Would you please add a few comments along the way helping explain things.
This PR is related to #1454 (comment)
cc @billonahill