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: withAndWithoutLongTermEnabled mod implementation #16180

Merged

Conversation

vtronkov
Copy link
Contributor

@vtronkov vtronkov commented Oct 25, 2024

Description:

  1. Converting defaultHapiSpec to hapiTest*
  2. Implementing AllTestsWithLongTermFlagEnabledTest and AllLeakyTestsWithLongTermFlagEnabledTest that consist of ALL_TEST list of test classes. In these new test classes we are getting all tests methods from ALL_TEST, we enable the scheduling.longTermEnabled flag and run the tests. That way we are not having a separate CI step and we are testing the scheduled tests with the flag enabled**
  3. Ignoring signFailsDueToDeletedExpiration test since it works only with longTermEnabled=false
  • *This was relevant for the first implementation but not anymore. I will leave the change because we are using hapiTest more frequently.
  • **I separated the tests to @HapiTests and @LeakyHapiTests because the leaky tests are running in series. In order to make it work I need to annotate runAllTests with @LeakyHapiTest which makes all ~110 tests running in series because of 5 tests that are leaky. If I combine all the tests it takes about 1:20 minutes(as embedded test) to run all tests. If they are separated it takes around 30 seconds.

Related issue(s):

Fixes #16010

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

The history of the different implementations

(just for reference/history, not required to read)
  1. My first idea was to make it work as _Eth suffix is working but this wasn't exactly how the mono's withAndWithoutLongTermEnabled was working. So I put it aside.
  2. Writing a jUnit extension to turn on the flag - I was unable to figure out a way to insert SpecOperation into the currently executed method. I didn't spend too much time here because I didn't quite like this solution
  3. Wrote my own HapiSpec.hapiTest method
    3.1 In The first version I was building two separate DynamicTest - one with the flag turned on and one with the flag turned off. Then I put the two tests into the returned Stream<DynamicTest>. This was kinda working but I had problems with the state and things didn't work the way I expected.
    3.2 In the second version I was building a single DynamicTest where I put the operation twice - with a flag operation turned off at the beginning and a flag operation turned on between the tests. Again I had some store problems, the ids were duplicated and I got errors along the lines of "the scheduled transaction was already executed". I tried fixing these problems by changing the names of the operations but in order to do that I had to make deep copies of the objects and it was getting more complicated than needed.
  4. In the end, I returned to the original idea - using the file name as an indicator that the flag should be turned on(and eventually adding additional logic).
  5. Because of the discussion with Michael(see below) and since we decided not to add the additional complex logic there is no need to implement the suffix solution. Thus the AllTestsWithLongTermFlagEnabledTest and AllLeakyTestsWithLongTermFlagEnabledTest was implemented(see details above)

My questions to @tinker-michaelj are:

  1. Are you okay with this approach or you had something else in mind? If so I'm guessing I will try to integrate this with CI just like _Eth was working
  2. Currently all tests from the schedule package instead of 2 are passing. In the beginning, we were talking about making this logic smart so it can detect that the flag is turned on and if needed to sleep until a scheduled transaction is executed. It looks like most of the tests are testing the transactions themselves and there are only a few end-to-end tests where this additional logic will be helpful. Maybe it's an overkill to add it at that point and it would be easier to write tests that are only for long-term transactions. Or maybe I'm missing something here?

Discussed this with Michael - we don't need the additional "understanding that the flag is on and make the test sleep until the transaction is executed" is not needed because the tests are not written in that way

JivkoKelchev and others added 28 commits October 16, 2024 10:13
Signed-off-by: Valentin Tronkov <[email protected]>
Signed-off-by: ibankov <[email protected]>
…transactions

# Conflicts:
#	hedera-node/hedera-app/src/main/java/com/hedera/node/app/workflows/handle/HandleWorkflow.java
Signed-off-by: Zhivko Kelchev <[email protected]>
refactor hapi tests

Signed-off-by: Zhivko Kelchev <[email protected]>
Signed-off-by: Michael Tinker <[email protected]>
Signed-off-by: Michael Tinker <[email protected]>
Signed-off-by: Michael Tinker <[email protected]>
Signed-off-by: Michael Tinker <[email protected]>
Signed-off-by: Michael Tinker <[email protected]>
Signed-off-by: Michael Tinker <[email protected]>
Signed-off-by: Michael Tinker <[email protected]>
Signed-off-by: Zhivko Kelchev <[email protected]>
Signed-off-by: Zhivko Kelchev <[email protected]>
Signed-off-by: Zhivko Kelchev <[email protected]>
fix long term schedule throttle warning.

Signed-off-by: Zhivko Kelchev <[email protected]>
…persistence' into 15055-POC-long-term-scheduled-transactions

# Conflicts:
#	hedera-node/hedera-schedule-service-impl/src/main/java/com/hedera/node/app/service/schedule/impl/handlers/HandlerUtility.java
#	hedera-node/hedera-schedule-service-impl/src/main/java/com/hedera/node/app/service/schedule/impl/handlers/ScheduleCreateHandler.java
#	hedera-node/hedera-schedule-service-impl/src/main/java/com/hedera/node/app/service/schedule/impl/handlers/ScheduleDeleteHandler.java
#	hedera-node/hedera-schedule-service-impl/src/main/java/com/hedera/node/app/service/schedule/impl/handlers/ScheduleManager.java
#	hedera-node/hedera-schedule-service-impl/src/main/java/com/hedera/node/app/service/schedule/impl/handlers/ScheduleSignHandler.java
#	hedera-node/hedera-schedule-service-impl/src/test/java/com/hedera/node/app/service/schedule/impl/handlers/HandlerUtilityTest.java
#	hedera-node/hedera-schedule-service-impl/src/test/java/com/hedera/node/app/service/schedule/impl/handlers/ScheduleCreateHandlerTest.java
#	hedera-node/hedera-schedule-service-impl/src/test/java/com/hedera/node/app/service/schedule/impl/handlers/ScheduleManagerTest.java
Signed-off-by: Zhivko Kelchev <[email protected]>
…transactions

# Conflicts:
#	hedera-node/hedera-schedule-service-impl/src/main/java/com/hedera/node/app/service/schedule/impl/handlers/AbstractScheduleHandler.java
#	hedera-node/hedera-schedule-service-impl/src/main/java/com/hedera/node/app/service/schedule/impl/handlers/HandlerUtility.java
#	hedera-node/hedera-schedule-service-impl/src/main/java/com/hedera/node/app/service/schedule/impl/handlers/ScheduleCreateHandler.java
#	hedera-node/hedera-schedule-service-impl/src/main/java/com/hedera/node/app/service/schedule/impl/handlers/ScheduleDeleteHandler.java
#	hedera-node/hedera-schedule-service-impl/src/main/java/com/hedera/node/app/service/schedule/impl/handlers/ScheduleSignHandler.java
Signed-off-by: Valentin Tronkov <[email protected]>
JivkoKelchev and others added 16 commits November 20, 2024 16:20
Signed-off-by: Zhivko Kelchev <[email protected]>
Signed-off-by: Zhivko Kelchev <[email protected]>
Signed-off-by: ibankov <[email protected]>
Signed-off-by: Zhivko Kelchev <[email protected]>
Signed-off-by: Zhivko Kelchev <[email protected]>
Signed-off-by: Zhivko Kelchev <[email protected]>
Signed-off-by: Michael Tinker <[email protected]>
Signed-off-by: Zhivko Kelchev <[email protected]>
Co-authored-by: Zhivko Kelchev <[email protected]>
Co-authored-by: Joseph S. <[email protected]>
Signed-off-by: Michael Tinker <[email protected]>
…into 16010_rehabilitate_mono_withAndWithoutLongTermEnabled_modifier

# Conflicts:
#	hedera-node/test-clients/src/main/java/module-info.java
Signed-off-by: Valentin Tronkov <[email protected]>
Base automatically changed from 15055-POC-long-term-scheduled-transactions to develop November 25, 2024 15:03
…dWithoutLongTermEnabled_modifier

# Conflicts:
#	hapi/hedera-protobufs/services/response_code.proto
#	hedera-node/hedera-app/src/jmh/java/com/hedera/node/app/blocks/BlockStreamManagerBenchmark.java
#	hedera-node/hedera-app/src/jmh/java/com/hedera/node/app/blocks/StandaloneRoundManagement.java
#	hedera-node/hedera-app/src/main/java/com/hedera/node/app/workflows/standalone/TransactionExecutors.java
Copy link
Collaborator

@tinker-michaelj tinker-michaelj left a comment

Choose a reason for hiding this comment

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

Nicely executed @vtronkov ! LGTM, tyvm 🙏

@vtronkov vtronkov merged commit d43a749 into develop Nov 27, 2024
49 of 50 checks passed
@vtronkov vtronkov deleted the 16010_rehabilitate_mono_withAndWithoutLongTermEnabled_modifier branch November 27, 2024 15:55
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.

Rehabilitate the mono withAndWithoutLongTermEnabled() HapiTest modifier
4 participants