Integration Tests For FeePerKb #2433

wilmerrootstock · 2024-06-01T22:39:18Z

Description

Making use of the newly created InMemoryStorage class, create tests simulating different real-life scenarios, avoiding using mocks as much as possible.

Integration tests are written to observe the behavior simulating real-life scenarios and stressing the InMemoryStorage executing them one after another.

Motivation and Context

Get Coverage with more realistic scenarios, even more than the Unit Tests.

How Has This Been Tested?

Through Integration tests.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Tests for the changes have been added (for bug fixes / features)
Requires Activation Code (Hard Fork)

Other information:

apancorb

Looks very nice and readable, left two comments

apancorb · 2024-06-03T08:57:21Z

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbIntegrationTest.java

+        assertFeePerKbValue(genesisFeePerKb);
+
+        /*
+         *  1st voting: authorizer 1 and 2 vote the same value


I am wandering, can each of these sections be placed in its own test? Or is there a need for all of them to be chained in this manner?

Hey, Antonio!
Although we're working with UT's tools, this approach creates integration tests, that is, test the whole Fee Per KB module, for this reason, it is important to make different test cases stressing the InMemoryStorage executing one test after another, and observe the behavior interacting like a real-life scenario without isolate them.

We should discuss this with the rest of the team @apancorb. The plan here was to create a test that simulates a real interaction with the fee per kb feature. That implies keeping the state and not reseting it between tests.

One option is to do it this way, in a single big test executing one action after the other. The other option is to do it in separate tests, sharing the same environment between tests. This last option implies that those tests will not be independent.

What do you think? Any other ideas?

I see, for integration tests sharing state between tests is fine. I would recommend having each Xth voting section in its own small test and sharing state at the class level.

Check this article out that will describe how to set per-class lifecycle and how to set the ordering of tests if it matters. This might help you achieve what you want to create.

As a side note, integration tests are usually placed under the intTest directory next to main and test. Gradle natively supports this convention. Maybe in the future this is something we can adopt.

I agree with @apancorb. This long test should be split using one of the options available for this case using junit.

Refactoring Test Integration for Fee Per KB

Adding some annotations such as @testinstance, and @BeforeAll to help share state in the test.

Adding annotations such as @TestMethodOrder, and @order to create an order of test execution.

Adding Caller enum to get the code more expressive.

After adding the Caller enum, adapt the voteFeePerKb method to receive as an In Parameter RskAddress.

Removing voteFeePerKbByUnauthorizedCaller, getAuthorizedRskAddresses, and getUnauthorizedRskAddress methods since they are no longer needed after the adaptation of the Caller enum in the code.

P.S.: it's just missing to move this integration test to the integrationTest directory. I'm solving some dependency issues.

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbIntegrationTest.java

jeremy-then · 2024-06-04T04:13:20Z

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbIntegrationTest.java

+    private RskAddress getUnauthorizedRskAddress() {
+        return new RskAddress("e2a5070b4e2cb77fe22dff05d9dcdc4d3eaa6ead");
+    }
+}


What happens if, for example, the current fee per kb is 100_000L and the authorizers decide to vote for the same 100_000L? Does it get rejected? Does it throw an error?

Hey, Jere!
Brilliant contribution! In this case, 100_000L is set again as "the new fee per KB value".
I just added this case to the tests:
16th voting: authorized callers vote the same current fee per KB value

nathanieliov

Very well done, Wilmer! Just a single comment, same as @apancorb: I would rather split the test into separate tests sharing the same context instance.

The base branch was changed.

marcos-iov · 2024-06-03T19:01:23Z

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbIntegrationTest.java

+        Coin differentFeePerKbVote = Coin.valueOf(60_000L);
+        Coin maxFeePerKbVote = feePerKbConstants.getMaxFeePerKb();
+        Coin excessiveFeePerKbVote = maxFeePerKbVote.add(Coin.SATOSHI);
+        Coin genesisFeePerKb = feePerKbConstants.getGenesisFeePerKb();


I think it's best if we declare each of these near the place where they are about to be used

marcos-iov · 2024-06-13T20:50:30Z

rskj-core/src/test/java/co/rsk/peg/utils/Caller.java

+ * Created to makes easier the way to get RskAddress of 3 authorized and 1 unauthorized Callers. It
+ * contains hex encoded string associated to each caller and return the RskAddress of each one.
+ */
+public enum Caller {


I wouldn't create this, at least not in a separate file. If it makes sense to have this enum then simply it put it inside the test class, it's the only place it's used

But, we could use it in the UTs as well. We can think about having an isolated library project in the future for tests and create a .jar to be used as a kind of test library/framework, that could be used in cross-projects (RSKj/Powpeg-Node) and cross-tests (UTs and ITs). In the meantime, we might put all these objects together in a utils package. Creating it inside the class breaks the Single Responsibility Principle.

I like @wilmerrootstock idea very much about moving this out of the test for the benefit of reusability.

Ok, in that case I'd move this to feeperkb package and rename it FeePerKbVoteCaller. What do you think?

I'm not a fan of generic util packages or classes

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbIntegrationTest.java

apancorb · 2024-06-20T08:28:57Z

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbIntegrationTest.java

+@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
+@TestInstance(Lifecycle.PER_CLASS)


Not sure, but I don't think the @TestInstance(Lifecycle.PER_CLASS) annotation is needed here since it is implied by @TestMethodOrder(MethodOrderer.OrderAnnotation.class)

Hey, @apancorb!
@TestMethodOrder(MethodOrderer.OrderAnnotation.class) doesn't have built-in @TestInstance(Lifecycle.PER_CLASS) in it. If we don't set @TestInstance(Lifecycle.PER_CLASS) it goes work Lifecycle.PER_METHOD by default. That means, if we are going to share a state of a variable in all methods, for instance, currentFeePerKb, it must be static. Apart from that, the method annotated with @BeforeAll must be static and all into it, static as well. That being said, I believe our best option is to use @TestInstance(Lifecycle.PER_CLASS).

apancorb · 2024-06-20T08:30:33Z

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbVoteCaller.java

+ * Created to makes easier the way to get RskAddress of 3 authorized and 1 unauthorized Callers. It
+ * contains hex encoded string associated to each caller and return the RskAddress of each one.
+ */
+public enum FeePerKbVoteCaller {


Are we expecting this to only be used by FeePerKbIntegrationTest? If that is the case, we can just declare them as constants in FeePerKbIntegrationTest

Hey, @apancorb! Take a look at this thread, this topic was discussed in it:

#2433 (comment)

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbIntegrationTest.java

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbIT.java

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbVoteCaller.java

lucasvuotto · 2024-06-25T14:34:29Z

pipeline: run

@testinstance

Making use of the newly created InMemoryStorage class, create tests simulating different real-life scenarios, avoiding using mocks as much as possible. Integration tests are written to observe the behavior simulating real-life scenarios and stressing the InMemoryStorage executing them one after another. White space before open brace - Adding white space before open brace to enhance the indentation. Improving Comments Adding 16th voting test - Authorized callers vote same current fee per KB value. Refactoring Test Integration for Fee Per KB - Adding some annotations such as @testinstance, and @BeforeAll to help sharing state in the test. - Adding annotations such as @TestMethodOrder, and @order to create an order of test execution. - Adding Caller enum to get the code more expressive. - After adding the Caller enum, adapt the voteFeePerKb method to receive as an In Parameter RskAddress. - Removing voteFeePerKbByUnauthorizedCaller, getAuthorizedRskAddresses, and getUnauthorizedRskAddress methods since they are no longer needed after the adaptation of the Caller enum in the code. Renaming and Moving Enum to Another Package - Renaming Enum from Caller to FeePerKbVoteCaller. - Moving Enum from utils to feeperkb package. Changing type of variable - Changing type of variable from instance to local. Renaming Test Methods - Renaming Test Methods to fit our method names for IT. - Removing JavaDocs for Methods since with new method names they are not needed anymore. Renaming Class Name - Renaming class name, to apply new naming conventions for Integration Tests(IT). This consists of adding at the end of the class name the suffix IT. Changing differentFeePerKbVote From Variable To Constant.

- Fixing naming convention for a variable name to use camel case. Improving Comment

@order

Improving Method Name for @order(8)

Removing Unnecessary Comment Removing Unnecessary Vote - Removing unnecessary vote since it doesn't help to go straight to the point to the sense of the test method.

sonarcloud · 2024-06-26T12:39:50Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

marcos-iov · 2024-06-26T14:12:35Z

pipeline:run

wilmerrootstock requested a review from a team as a code owner June 1, 2024 22:39

apancorb reviewed Jun 3, 2024

View reviewed changes

jeremy-then reviewed Jun 4, 2024

View reviewed changes

nathanieliov previously approved these changes Jun 6, 2024

View reviewed changes

marcos-iov force-pushed the fee-per-kb-refactor-integration branch from 7fc0436 to f1edb96 Compare June 6, 2024 15:49

marcos-iov force-pushed the feature/integration-tests-fee-per-kb branch from 7f85689 to 4804699 Compare June 7, 2024 14:00

Base automatically changed from fee-per-kb-refactor-integration to master June 10, 2024 13:20

wilmerrootstock force-pushed the feature/integration-tests-fee-per-kb branch from 4804699 to 9a0297e Compare June 13, 2024 04:34

marcos-iov reviewed Jun 13, 2024

View reviewed changes

wilmerrootstock force-pushed the feature/integration-tests-fee-per-kb branch 2 times, most recently from 87b8496 to 058d47d Compare June 18, 2024 21:59

julia-zack reviewed Jun 19, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbIntegrationTest.java Outdated Show resolved Hide resolved

apancorb reviewed Jun 20, 2024

View reviewed changes

nathanieliov reviewed Jun 21, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbIntegrationTest.java Outdated Show resolved Hide resolved

marcos-iov force-pushed the feature/integration-tests-fee-per-kb branch from b982a47 to f4e06f2 Compare June 21, 2024 19:42

marcos-iov previously approved these changes Jun 21, 2024

View reviewed changes

apancorb previously approved these changes Jun 24, 2024

View reviewed changes

julia-zack reviewed Jun 24, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbIT.java Show resolved Hide resolved

julia-zack reviewed Jun 24, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbIT.java Outdated Show resolved Hide resolved

julia-zack reviewed Jun 24, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbIT.java Outdated Show resolved Hide resolved

julia-zack reviewed Jun 24, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbIT.java Outdated Show resolved Hide resolved

julia-zack reviewed Jun 24, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/feeperkb/FeePerKbVoteCaller.java Outdated Show resolved Hide resolved

wilmerrootstock dismissed stale reviews from apancorb and marcos-iov via 6da6299 June 24, 2024 15:05

marcos-iov force-pushed the feature/integration-tests-fee-per-kb branch from 1831bde to b72f37a Compare June 25, 2024 15:51

marcos-iov approved these changes Jun 25, 2024

View reviewed changes

julia-zack self-requested a review June 25, 2024 17:42

nathanieliov approved these changes Jun 26, 2024

View reviewed changes

wilmerrootstock and others added 6 commits June 26, 2024 09:34

Integration Tests for FeePerKb

d2b8117

Create a first integration test

e8a07ea

Fixing Naming Convention For A Variable Name

77666c7

- Fixing naming convention for a variable name to use camel case. Improving Comment

Improving Method Name

b0faaed

Improving Method Name for @order(8)

Adding a comment to make clearer the use of currentFeePerKb variable.

6f62827

Removing Unnecessary Comment Removing Unnecessary Vote - Removing unnecessary vote since it doesn't help to go straight to the point to the sense of the test method.

marcos-iov force-pushed the feature/integration-tests-fee-per-kb branch from b72f37a to 6f62827 Compare June 26, 2024 12:36

josedahlquist merged commit 2bbedf5 into master Jun 26, 2024
10 checks passed

marcos-iov deleted the feature/integration-tests-fee-per-kb branch June 26, 2024 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration Tests For FeePerKb #2433

Integration Tests For FeePerKb #2433

wilmerrootstock commented Jun 1, 2024

apancorb left a comment

apancorb Jun 3, 2024

wilmerrootstock Jun 3, 2024

marcos-iov Jun 3, 2024 •

edited

Loading

apancorb Jun 4, 2024

nathanieliov Jun 6, 2024

wilmerrootstock Jun 13, 2024 •

edited

Loading

jeremy-then Jun 4, 2024

wilmerrootstock Jun 4, 2024

nathanieliov left a comment

marcos-iov Jun 3, 2024

wilmerrootstock Jun 18, 2024

marcos-iov Jun 13, 2024

wilmerrootstock Jun 13, 2024

nathanieliov Jun 13, 2024

marcos-iov Jun 14, 2024

wilmerrootstock Jun 18, 2024

apancorb Jun 20, 2024 •

edited

Loading

wilmerrootstock Jun 21, 2024 •

edited

Loading

apancorb Jun 21, 2024

apancorb Jun 20, 2024

wilmerrootstock Jun 21, 2024

lucasvuotto commented Jun 25, 2024

sonarcloud bot commented Jun 26, 2024

marcos-iov commented Jun 26, 2024

		@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
		@TestInstance(Lifecycle.PER_CLASS)

Integration Tests For FeePerKb #2433

Integration Tests For FeePerKb #2433

Conversation

wilmerrootstock commented Jun 1, 2024

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

apancorb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcos-iov Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilmerrootstock Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Refactoring Test Integration for Fee Per KB

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanieliov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apancorb Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

wilmerrootstock Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasvuotto commented Jun 25, 2024

sonarcloud bot commented Jun 26, 2024

Quality Gate passed

marcos-iov commented Jun 26, 2024

marcos-iov Jun 3, 2024 •

edited

Loading

wilmerrootstock Jun 13, 2024 •

edited

Loading

apancorb Jun 20, 2024 •

edited

Loading

wilmerrootstock Jun 21, 2024 •

edited

Loading