phet commented on code in PR #3944:
URL: https://github.com/apache/gobblin/pull/3944#discussion_r1591507977
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/OrchestratorTest.java:
##########
@@ -81,12 +84,15 @@ public class OrchestratorTest {
private FlowCatalog flowCatalog;
private FlowSpec flowSpec;
- private Orchestrator orchestrator;
+
+ private FlowStatusGenerator mockFlowStatusGenerator;
+ private DagManager mockDagManager;
+ private Orchestrator dagMgrNotFlowLaunchHandlerBasedOrchestrator;
private static final String TEST_USER = "testUser";
private static final String TEST_PASSWORD = "testPassword";
private static final String TEST_TABLE = "quotas";
- @BeforeClass
+ @BeforeMethod
Review Comment:
unfortunately lots of "testing smells" here.
first off, class-level test init doesn't play well w/ verifying mock
interactions, since we want a fresh count for the exec of each test method.
relatedly it also complicates - if not outright foreclosing on - parallel test
method execution.
secondly, the `@Test(dependsOnMethod = ...)` structuring is a testing
anti-pattern that here obscured that `setup`-style init had been formulated
instead as test methods; see - `createTopologySpec()`. a major clue of
something wrong is that particular "@Test" method does not even exercise
`Orchestrator`, our class-under-test!
for now, I've fixed this suite to use per-method setup/teardown best
practices, but left it as a TODO to figure out where `createTopologySpec`
actually belongs. `createFlowSpec` and `deleteFlowSpec` also deserve attention
in this same regard.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
