Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRAFT: Refactoring RPCWithMemoTest so to make easier to re-implement in Go #172

Closed
wants to merge 1 commit into from

Conversation

edmondop
Copy link

@edmondop edmondop commented Jun 12, 2023

This PR comes as a consequence of trying to implement indeedeng/iwf-golang-sdk#64. After completing the model, I had to migrate the tests and it was very hard to make sense of the existing one.

Overall, I propose the following improvements:

  • Break down each step that performs multiple assertions into different tests, so to have independent and isolated failures
  • Avoid Thread.sleep in tests and delegate eventuay consistency matching logic to a library, in this case awaitility
  • Use Hamcrest matchers and their combinatorial logic to create other matchers
  • Make better usage of JUnit behavior, in particular that each tests will be run on a separate instance of the test class, to do more initialization in the @BeforeEach rather than at the beginning of each test
  • Use generics to avoid repetition for the tests that set and get the attributes and keyword properties
  • Added thread-safety in gray areas for singleton

TODO:

  • Port the readonly test
  • Fix the two tests that fail io.iworkflow.integ.RpcWithMemoTest#testRpcFunc0OutputIsCorrect, io.iworkflow.integ.RpcWithMemoTest#testRpcFunc1WorkflowResultIsCorrect both failing for this workflow should have one or zero state output for using this API, found 2, after filtered NULL: 0

Further improvement down the line:

  • Use dynamic generation of tests using the @TestFactory JUnit annotation so to automatically generate the testcases for Func1, Func0 and Proc1/Proc0
  • Move integration tests from a package to a separate source folder, as per Java conventions

@edmondop edmondop changed the title Splitting assertions in test cases DRAFT: Refactoring RPCWithMemoTest so to make easier to re-implement in Go Jun 12, 2023
Copy link
Contributor

@longquanzheng longquanzheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like the tests are failing.

@@ -27,7 +27,7 @@ public CommandRequest waitUntil(
}

@Override
public StateDecision execute(
public synchronized StateDecision execute(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing that

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like the tests are failing.

Yes, that's why is in Draft. I was looking for help to understand why the two tests are failing :)


// search attrs
public void assertSearchAttributesAreCorrect(String expectedSearchAttributeKeyword){
final Map<String, Object> searchAttributes = client.getWorkflowSearchAttributes(BasicPersistenceWorkflow.class,
wfId, "", Arrays.asList(TEST_SEARCH_ATTRIBUTE_KEYWORD, TEST_SEARCH_ATTRIBUTE_INT));

Assertions.assertEquals(ImmutableMap.builder()
.put(TEST_SEARCH_ATTRIBUTE_INT, RPC_OUTPUT)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this rpc doesn't mutate the TEST_SEARCH_ATTRIBUTE_INT and the int doens't work for string

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants