Get ExecGraph hosts
#178
Merged
Get ExecGraph hosts
#178
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Shillaker
requested changes
Nov 22, 2021
| @@ -72,6 +72,25 @@ TEST_CASE("Test execution graph", "[scheduler][exec-graph]") | |||
| checkExecGraphEquality(expected, actual); | |||
| } | |||
|
|
|||
| TEST_CASE("Test execution graph node count", "[scheduler][exec-graph]") | |||
There was a problem hiding this comment.
This is very nitpicky but the API is about getting the unique host names from the graph, not the count.
I'd have thought this test would be called Test get unique hosts from exec graph, then the assertion at the end would look like:
std::set<std::string> expected = {"foo", "bar", "baz"};
REQUIRE(hosts == expected);Currently the test also doesn't cover duplicates, it's got 3 messages each executed on a different host. It would be good to add another message for a different function but executed on the same host as one of the others to be extra sure.
There was a problem hiding this comment.
All good points, thanks.
Shillaker
approved these changes
Nov 23, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add utility function to get the distinct hosts in an execution graph.
Also bump the code version to re-build the containers as required in #177