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

Allow WorkflowImplementationOptions to be passed in TestWorkflowExten… #1948

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

picpromusic
Copy link
Contributor

@picpromusic picpromusic commented Dec 4, 2023

What was changed

It is possible to configure WorkflowImplementationsOptions using TestWorkflowExtension.
#1626

Why?

In order to configure WorkflowImplementationOptions (like default ActivityOptions) using TestWorkflowExtension two new Methods registerWorkflowImplementationTypes are implemented. I choosed registerWorkflowImplementationTypes and not setWorkflowTypes because I found the registerWorkflowImplementationTypes like the one needed to configure WorkflowImplementationOptions to a worker in "non-test" Code.

Checklist

  1. Closes 1626

  2. How was this tested:
    With my tests in a different project and through 2 Unit-Tests. One (TestWorkflowImplementationOptions.java) providing WorkflowImplementationOptions and one not providing the options (which would run into end endless loop). I did not find a more direct-way to tests this. So just testing side-effects of the implementation. An because side-effects might change in future TestWorkflowImplementationOptionsViceVersa.java for testing that TestWorkflowImplementationOptions.java would brake if the side-effect changes.

  3. Any docs updates needed?
    I did not find a documentation that needs to be adapted. But https://github.com/temporalio/documentation/blob/main/docs-src/java/testing-frameworks.md should be fixed, It contains a broken link to https://github.com/temporalio/samples-java/tree/main/src/test/java/io/temporal/samples which should be https://github.com/temporalio/samples-java/tree/main/core/src/main/java/io/temporal/samples. As it is a different repository. I will open up another pull-request for it.

@CLAassistant
Copy link

CLAassistant commented Dec 4, 2023

CLA assistant check
All committers have signed the CLA.

@picpromusic picpromusic marked this pull request as ready for review December 4, 2023 14:01
@picpromusic picpromusic requested a review from a team as a code owner December 4, 2023 14:01
@Quinn-With-Two-Ns
Copy link
Contributor

Hi @picpromusic , thank you for the contribution. Just had one small comment on the content. For testing I think your approach is correct, but I would prefer to see setFailWorkflowExceptionTypes tested over over any timeout test as those can be flaky in CI

@Quinn-With-Two-Ns
Copy link
Contributor

Just an FYI the CI failure looks unrelated to your changes

@picpromusic
Copy link
Contributor Author

@Quinn-With-Two-Ns. And how to continue?

@Quinn-With-Two-Ns
Copy link
Contributor

@picpromusic I'll need to merge this #1949 and then you can rebase on this.

@Quinn-With-Two-Ns
Copy link
Contributor

@picpromusic CI should be fixed https://buildkite.com/temporal/java-sdk-public/builds/2979 please feel free to rebase

…sion

Implement registerWorkflowImplementationTypes for TestWorkflowExtension temporalio#1626
Fix use of deprecated Method in existing tests.
Testing the setting indirectly by specifying Default startToCloseTimeout or not.
…sion

use setFailWorkflowExceptionTypes instead of timeout-setting
@Quinn-With-Two-Ns Quinn-With-Two-Ns merged commit bb43d37 into temporalio:master Dec 6, 2023
6 checks passed
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.

Allow WorkflowImplementationOptions to be passed in TestWorkflowExtension->setWorkflowTypes
3 participants