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

feat(environment): create the EZE environment for spec tests #186

Closed
wants to merge 3 commits into from

Conversation

aivinog1
Copy link

This is the first part of #100.

I just create the EZE environment which consists of EZE as Zeebe part and ZeeQS in the Docker as query service. We could improve it in the future but this is for start.

@aivinog1 aivinog1 requested a review from saig0 September 20, 2022 06:26
@aivinog1 aivinog1 self-assigned this Sep 20, 2022
@saig0
Copy link
Collaborator

saig0 commented Sep 20, 2022

@aivinog1 thank you for your contribution. 🎉

Currently, I'm busy and don't have time to review your changes (we are short before the Zeebe release). I'll have a look at least in two weeks. 👀

@aivinog1
Copy link
Author

Currently, I'm busy and don't have time to review your changes (we are short before the Zeebe release). I'll have a look at least in two weeks. 👀

Sure, no problem :) If you need some help, you can ask me :)

Copy link
Collaborator

@saig0 saig0 left a comment

Choose a reason for hiding this comment

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

@aivinog1 great contribution. 🎉 In general, I like the changes. 👍

One minor comment: the ZeebeTestRunnerTest is taking too long (7 minutes on my laptop). The parameterized tests are a good idea but the execution is too long. I would be find with a smaller subset of tests to verify the correctness.

One major comment: I'm not sure about the splitting of the environment. It's working but it feels not the right abstraction. I think that a new test runner for EZE would be a better option with more flexibility. We could extract the common parts (ZeeQS client) into separate modules.

Do you agree? Do you want to make the change?

If not, it's completely fine. I would accept the PR and do the other change by myself.

@aivinog1
Copy link
Author

Hey @saig0! Sure, I will try to fix it :)

@aivinog1 aivinog1 requested a review from saig0 October 26, 2022 06:28
@aivinog1
Copy link
Author

Hey @saig0! Can you check this out, please? :)

@aivinog1
Copy link
Author

aivinog1 commented Nov 7, 2022

@saig0 Hey! Can you take a look at this, please? :)

@saig0
Copy link
Collaborator

saig0 commented Nov 8, 2022

@aivinog1 yes! I'm sorry but the delay. 🙇
We got a lot of great contributions during Hacktoberfest. I'm still in the process of reviewing them. 😅 I'll have a look at your PR as soon as possible.

@aivinog1
Copy link
Author

Hey @saig0! This is a kind reminder about this PR :)

@saig0
Copy link
Collaborator

saig0 commented Dec 16, 2022

@aivinog1 I'm sorry about the delay. 🙇 I'll look into your PR next week.

Copy link
Collaborator

@saig0 saig0 left a comment

Choose a reason for hiding this comment

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

@aivinog1 I had a look at your changes. Thank you for all your effort. 🍰 👍

I'm still struggling a bit with the structure. To get a better idea, of what I would like to have I'll code a bit myself over the holidays. I will probably use some of your code.

I'll keep you updated.

@saig0
Copy link
Collaborator

saig0 commented Jan 4, 2023

I'm closing this PR in favor of #212.

The PR #212 adds a new test runner for EZE next to the existing test runner for Zeebe. Instead of ZeeQS, the test runner uses the records directly from EZE to collect the state of the process instances. Using only EZE reduces the startup time to less than a second. 🚀

@aivinog1 thank you for your contribution and for sharing your ideas. 🙇

@saig0 saig0 closed this Jan 4, 2023
@aivinog1 aivinog1 deleted the eze-test-runner branch January 7, 2023 05:58
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