Remove flaky test using Testcontainers and a more specific test approach #1288
Conversation
|
@mcruzdev what's the idea here? just to replicate the same tests (the flaky one) but with Testcontainers and Toxiproxy managed by testcontainers? |
The idea is to use Testcontainers and to test in a different way. |
|
As soon as it test the same thing, we can use this to create a plan to move all tests to use test containers |
I changed the test to be more specific, testing each configuration separated. |
mcruzdev
changed the title
mcruzdev
force-pushed
the
issue-1258
branch
3 times, most recently
from
7d5fa66 to
928feb5
Compare
|
@artur-ciocanu did you had time to check this out? This is definitely our first step to move our integration tests to testcontainers .. and it is looking really good IMO. Do you have any concerns about this ? |
I agree, this is a great start and will be great in the long run for standardization across IT tests. @salaboy - Please cc the listed java approvers + maintainers for input as opposed to individual GH handles, as that way, when we add more approvers and maintainers we don't leave anyone out. |
There was a problem hiding this comment.
@mcruzdev awesome work, love it! I have left some comments could you please address them and I'll take another look.
Thank you ❤️
| import static com.github.tomakehurst.wiremock.client.WireMock.urlMatching; | ||
| import static com.github.tomakehurst.wiremock.client.WireMock.verify; | ||
| import static io.dapr.it.resiliency.SdkResiliencyIT.WIREMOCK_PORT; | ||
| import static io.dapr.it.testcontainers.DaprContainerConstants.IMAGE_TAG; |
There was a problem hiding this comment.
Could we use a simpler import like:
import static org.assertj.core.api.Assertions.assertThat;
| private String appName; | ||
| private Integer appPort; | ||
| private String appHealthCheckPath; | ||
| private boolean shouldReusePlacement; |
There was a problem hiding this comment.
It is not clear, why we need this change. As far as I saw in the IT tests, the network alias is not required. I am not against enhancing DaprContainer with network alias, but I think it can be done in a separate PR where we could explain what is the use case and probably and a few unit tests to improve coverage.
There was a problem hiding this comment.
It is necessary because the Toxiproxy container needs to communicate with Dapr in the same network, it is more easy to do with aliases;
There was a problem hiding this comment.
@mcruzdev I see that you are using .withNetwork(NETWORK) for ToxiproxyContainer, hence my question regarding network alias.
As I mentioned we can add support for network alias in a separate PR and add the necessary code coverage to DaprContainer. Once we have support for network alias we can revisit this IT and use network alias instead of network.
There was a problem hiding this comment.
I got it, I changed the code :)
|
@cicoyle how do you do that ? Meaning mentioning all maintainers ? |
yes, as I want to ensure the appropriate folk are not left out. Here are the tags for the appropriate community members in this SDK: @dapr/approvers-java-sdk and @dapr/maintainers-java-sdk. But tagging is not necessary immediately after opening PRs and issues bc those groups already get emails about it bc approvers and maintainers are already listed as reviewers automatically I would say tagging is more for after a day or more goes by without review or input. |
|
@mcruzdev ok.. it seems that only codecov is missing, this is quite important so we can maintain the coverage over time. Can you check the report and see if you can cover more paths to get over the line? |
mcruzdev
force-pushed
the
issue-1258
branch
3 times, most recently
from
04745f3 to
acba370
Compare
Signed-off-by: Matheus Cruz <[email protected]>
There was a problem hiding this comment.
@mcruzdev thanks for addressing all the comments, it looks great!
...nfigure/src/test/java/io/dapr/spring/boot/autoconfigure/client/DaprClientPropertiesTest.java
Show resolved
Hide resolved
Signed-off-by: Matheus Cruz <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1288 +/- ##
============================================
- Coverage 76.91% 75.47% -1.44%
- Complexity 1592 1616 +24
============================================
Files 145 193 +48
Lines 4843 5101 +258
Branches 562 555 -7
============================================
+ Hits 3725 3850 +125
- Misses 821 935 +114
- Partials 297 316 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Please explain the changes you've made
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #[issue number]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: