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.

Sign up for GitHub

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

Jump to bottom

feat: get engine time #447

Closed
wants to merge 2 commits into from
Closed

feat: get engine time #447

wants to merge 2 commits into from

Conversation

jasoncwik
Copy link

Description

Implement a new API for the test engine that allows the test writer to query
the current time of the running engine. This helps to create tests that set
a specific time-of-day, especially for longer-running tests where there may
be multiple changes in the engine clock.

Related issues

closes #446

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually

Documentation:

  • Javadoc has been written
  • The documentation is updated

feat: get engine time
26daa97 
Implement a new API for the test engine that allows the test writer to query
the current time of the running engine. This helps to create tests that set
a specific time-of-day, especially for longer-running tests where there may
be multiple changes in the engine clock.
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@remcowesterhoud
Copy link
Contributor

remcowesterhoud commented Jul 19, 2022 •
edited
Loading

Thanks for your PR! I’ll have a look at it tomorrow.

in the meantime I have started the CI for it. Please also sign the CLA.

@remcowesterhoud remcowesterhoud self-requested a review July 19, 2022 17:56
@github-actions
Copy link

github-actions bot commented Jul 19, 2022 •
edited
Loading

Unit Test Results

  47 files    47 suites   2m 7s ⏱️
112 tests 112 ✔️ 0 💤 0 ❌
359 runs  359 ✔️ 0 💤 0 ❌

Results for commit 656698a.

♻️ This comment has been updated with latest results.

@jasoncwik
Copy link
Author

jasoncwik commented Jul 19, 2022 •
edited
Loading

Please also sign the CLA.

Will do. I just sent it off to our legal department for review.

remcowesterhoud
remcowesterhoud requested changes Jul 20, 2022
View reviewed changes
Copy link
Contributor

@remcowesterhoud remcowesterhoud left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks a lot for the PR @jasoncwik. I think this a great improvement and I would love to see this merged 👍

I have a few suggestions to get the PR ready to be merged. Please have a look at my comments and feel free to let me know if you disagree on something 😄 Overall they should all be quick-fixes.

During the review I have used emoji code

...src/main/java/io.camunda.zeebe.process.test.extension.testcontainer/ContainerizedEngine.java Outdated Show resolved Hide resolved
api/src/main/java/io/camunda/zeebe/process/test/api/ZeebeTestEngine.java Outdated Show resolved Hide resolved
engine-protocol/src/main/proto/engine_control.proto Outdated Show resolved Hide resolved
engine-protocol/src/main/proto/engine_control.proto Outdated Show resolved Hide resolved
engine/src/test/java/io/camunda/zeebe/process/test/engine/EngineClientTest.java Outdated Show resolved Hide resolved
feat: get engine time
656698a 
Updated PR based on review comments.
@remcowesterhoud remcowesterhoud self-requested a review July 20, 2022 20:57
remcowesterhoud
remcowesterhoud approved these changes Jul 21, 2022
View reviewed changes
Copy link
Contributor

@remcowesterhoud remcowesterhoud left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! 🚀

I have 1 more small comment, please have a look at it. Once you've heard back from your legal team and signed the CLA we can go ahead with the merge 🙂

...src/main/java/io.camunda.zeebe.process.test.extension.testcontainer/ContainerizedEngine.java Show resolved Hide resolved
@remcowesterhoud
Copy link
Contributor

Hey @jasoncwik. I was wondering if you had already heard back from your legal department. The CLA is the last thing that prevents me from merging this. It would be a shame to let it go to waste 🙂

@jasoncwik
Copy link
Author

Hey @jasoncwik. I was wondering if you had already heard back from your legal department. The CLA is the last thing that prevents me from merging this. It would be a shame to let it go to waste 🙂

Should have the CLA this week. Just working through some process changes on our side.

@remcowesterhoud
Copy link
Contributor

remcowesterhoud commented Sep 30, 2022 •
edited
Loading

Closing due to inactivity. Please re-open if you still want to work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@remcowesterhoud remcowesterhoud remcowesterhoud approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add feature to query the Zeebe engine's current time
3 participants
@jasoncwik @CLAassistant @remcowesterhoud