Skip to content

Migrate Redis integration tests to Testcontainers #3840

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
Closed
artembilan opened this issue Jul 8, 2022 · 6 comments
Closed

Migrate Redis integration tests to Testcontainers #3840

artembilan opened this issue Jul 8, 2022 · 6 comments
Labels
in: redis type: enhancement
Milestone
6.0.0-M4

Comments

@artembilan
Copy link
Member

Currently our integration tests rely on the Redis server provided by the host of the test execution and use JUnit 4 rule for condition evaluation.
We cannot have externally configured Redis server on CI environment anymore, so SonarQube reports pure coverage, since most of Redis tests are ignored.

Switch integration tests for Redis to Testcontainers since Docker is available in most modern environments.
This way we also will be able to migrate all those tests to JUnit 5.

See MosquittoContainerTest for example.
@artembilan artembilan added this to the Backlog milestone Jul 8, 2022
@oxcafedead
Copy link
Contributor

I will take a look if no objections, there is one more interesting "bug" to deal with.

@artembilan
Copy link
Member Author

Thank you, @oxcafedead !

Would you mind sharing what is that bug? Maybe it worth to raise a GH issue on the matter?

@oxcafedead
Copy link
Contributor

oxcafedead commented Jul 13, 2022 •
edited
Loading

@artembilan it should go away after the migration.
But anyway, the bug is that if tests extend RedisAvailableTests and test method does not have the annotation RedisAvailable - it will be always marked as passed even if it fails. I have found such test: RedisLockRegistryTests#testUnlink

It fails after my migration, so I disabled it. Probably the original author should take a look at it eventually because I don't understand how this test is intended to work (besides the typo in the test name and assertions, it operates with non-used mocks in the 'given' stage).

In main branch, this test is marked as 'passed' with almost immediate timing. This is because inside org/springframework/integration/redis/rules/RedisAvailableRule.java:91, base.evaluate(); does not happen in case when the annotation is not present.

@artembilan
Copy link
Member Author

Thank you!
The test is indeed wrong: it does not reflect the current logic around UNLINK command in the RedisLockRegistry.
I'll remove it shortly, so you won't need to worry about this test in your branch.

artembilan added a commit that referenced this issue Jul 13, 2022
@artembilan
Remove ulinkAvailable test for RedisLockRegistry
79d31b2 
The logic of the test does not reflect the state of the
`RedisLockRegistry` around `ulinkAvailable` property:
we don't check the Redis version anymore, but try to call
`UNLINK` command.
Then we reset `ulinkAvailable` to `false` to call regular `DEL`
for the rest of `RedisLockRegistry` life

The `RedisAvailableRule` marks `testUlink()` as passed even without
calling it.
We are not going to look into fixing the rule since we are migrating
to Testcontainers: #3840
@artembilan
Copy link
Member Author

Here it is: 79d31b2

@oxcafedead
Copy link
Contributor

Ok, I will rebase then, I hope there will be not too many conflicts :)

oxcafedead added a commit to oxcafedead/spring-integration that referenced this issue Jul 14, 2022
@oxcafedead
spring-projectsGH-3840: Migrate Redis tests to Testcontainers
5b3519a 
Fixes spring-projects#3840

* Create a new abstraction `RedisTest` for Testcontainers-based tests
* Move existing common utility methods to the new interface
* Migrate all existing Redis tests to use this new `RedisTest` interface
* Migrate all existing Redis tests to JUnit5
* Make all existing Redis tests confirming to JUnit5 standards, such as
default methods and classes visibility instead of public
* Add a dependency for parametrized tests in JUnit5
* Improve assertions readability across tests, for instance:
assertThat(thing.size()).isEqualTo(2) -> assertThat(thing).hasSize(2)
* Add real assertions to
`RedisLockRegistryTests.twoRedisLockRegistryTest` (earlier it did not
have assertions at all)
* Reformat, rearrange and cleanup the code
@oxcafedead oxcafedead mentioned this issue Jul 14, 2022
Closed
@artembilan artembilan modified the milestones: Backlog, 6.0.0-M4 Jul 14, 2022
oxcafedead added a commit to oxcafedead/spring-integration that referenced this issue Jul 14, 2022
@oxcafedead
spring-projectsGH-3840: Migrate Redis tests to Testcontainers
09d6662 
Fix a couple of small changes after the code review:
* Change base interface name to be consistent with other similar places
* Typo in javadocs
oxcafedead added a commit to oxcafedead/spring-integration that referenced this issue Jul 15, 2022
@oxcafedead
spring-projectsGH-3840: Migrate Redis tests to Testcontainers
3cedf76 
Small tests readability improvement regarding assertions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
in: redis type: enhancement
Projects
None yet
Milestone
6.0.0-M4
Development

Successfully merging a pull request may close this issue.

GH-3840: Migrate Redis tests to Testcontainers oxcafedead/spring-integration
2 participants
@artembilan @oxcafedead