Will-Lo commented on a change in pull request #3238:
URL: https://github.com/apache/gobblin/pull/3238#discussion_r586812283
##########
File path:
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -301,6 +302,42 @@ public void testLocalGroupOwnershipUpdates() throws
Exception {
_client.deleteFlowConfig(new
FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_7));
}
+
+ @Test (expectedExceptions = RestLiResponseException.class)
Review comment:
I think it would be a better practice for us to try and catch the
exception, and then assert that the error code is as expected. For example,
with `testGroupRequesterRejected`
```
try {
...
} catch (RestLiResponseException e) {
Assert.assertEquals(e.getStatus(), HttpStatus.ORDINAL_401_Unauthorized);
}
```
I was able to catch a bug before in this test suite as it actually returned
a different error code than expected. Also these tests would mark 500 errors as
passing tests which I think we should avoid.
Also it allows better granularity in the test too, so we can assert that
only the second call here would fail
##########
File path:
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -301,6 +302,42 @@ public void testLocalGroupOwnershipUpdates() throws
Exception {
_client.deleteFlowConfig(new
FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_7));
}
+
+ @Test (expectedExceptions = RestLiResponseException.class)
+ public void testGroupUpdateRejected() throws Exception {
+ ServiceRequester testRequester = new ServiceRequester("testName",
"USER_PRINCIPAL", "testFrom");
+ _requesterService.setRequester(testRequester);
+ Map<String, String> flowProperties = Maps.newHashMap();
+
+ FlowConfig flowConfig = new FlowConfig().setId(new
FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_8))
+ .setTemplateUris(TEST_TEMPLATE_URI)
+ .setProperties(new StringMap(flowProperties))
+ .setOwningGroup("testGroup");
+
+ _client.createFlowConfig(flowConfig);
+
+ flowConfig.setOwningGroup("testGroup2");
+ _client.updateFlowConfig(flowConfig);
Review comment:
We should avoid using testGroup2 as a failing group. There's another
test that writes testGroup2 to the file where the group ownership service reads
from, to test if the groupownership service will update automatically on reads,
and this could lead to flakiness
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
