Rework tests #19614
Rework tests #19614
Conversation
Blackbaud-MikeLueders
requested review from
backwind1233,
chenrujun,
jialindai,
kushagraThapar,
mbhaskar,
saragluna,
stliu and
yiliuTo
as code owners
ghost
added
Cosmos
azure-spring
|
Thank you for your contribution Blackbaud-MikeLueders! We will review the pull request and get back to you soon. |
|
@kushagraThapar This PR is not ready to be merged but I wanted to get this out there for feedback before I convert the rest of the integration tests. |
Makes sense, I will review the PR today. |
| // Licensed under the MIT License. | ||
| package com.azure.spring.data.cosmos; | ||
|
|
||
| import com.azure.cosmos.implementation.guava25.base.Stopwatch; |
There was a problem hiding this comment.
We should not use any implementation level details from azure-cosmos SDK. As those are internal details, and can be removed anytime. Can we please use apache-commons Stopwatch instead ? As spring-data-cosmos already takes a dependency on that.
There was a problem hiding this comment.
I figured it didn't matter since this was a test class and not exposed to anything outside of this project. The apache-commons Stopwatch is much older than the guava implementation and doesn't return a Duration so interacting with it is much less elegant - that said, the usage is fairly limited so not really a problem, will get it updated.
There was a problem hiding this comment.
yeah, I agree, guava is much better, but unfortunately, it is not one of the approved dependencies for azure-sdk-for-java. For smaller usage, I guess apache-commons is fine, but if a time comes where we need it more, we can try to internally shade it.
|
|
||
| public class ContainerLock { | ||
|
|
||
| private CosmosTemplate template; |
There was a problem hiding this comment.
This works for CosmosTemplate - do you plan to include ReactiveCosmosTemplate in the same ContainerLock class, or would you prefer for a ReactiveContainerLock class as we update more tests ?
There was a problem hiding this comment.
CosmosTempate is always available, even in the reactive tests (and many reactive tests already inject CosmosTemplate), so I just standardized on that to keep this class simple. I can add support for ReactiveCosmosTemplate as well as there are a few reactive tests that do not already pull in CosmosTemplate.
There was a problem hiding this comment.
Sounds good, so what it seems like is this will be case to case basis. If we need reactive cosmos template, we will pull it in.
| private ContainerLock otherLock; | ||
|
|
||
| @Before | ||
| public void setup() { |
There was a problem hiding this comment.
As far as I know, the @Before and @After will be called for each individual test in this class. If that's the case, then we will be creating and deleting containers for each test method. Can you please verify that it runs only once, and we only create and delete container only once per each test class, or per each test suite execution ?
There was a problem hiding this comment.
That's correct, @Before and @After will be called for each test method - but setup calls createContainerIfNotExists (which only creates the container once) while cleanup calls releaseLockIgnoreException (which doesn't delete teh container). There is a method afterClassCleanup which is annotated with @AfterClass which only executes once.
There was a problem hiding this comment.
Makes sense, thanks for the explanation.
|
|
||
| @AfterClass | ||
| public static void afterClassCleanup() { | ||
| staticTemplate.deleteContainer(new CosmosEntityInformation<>(ContainerLock.LockEntry.class).getContainerName()); |
There was a problem hiding this comment.
in the @After API above, container will be automatically deleted, no ?
If so, whey do we need the afterClassCleanup ?
There was a problem hiding this comment.
@After releases the lock and releasing the lock just deletes the lock document, it does not delete the container.
|
|
||
| @AfterClass | ||
| public static void afterClassCleanup() { | ||
| staticTemplate.deleteContainer(entityInformation.getContainerName()); |
There was a problem hiding this comment.
Since we have removed these two APIs, @After and @AfterClass - who will handle the data and container delete operation for this integration test?
There was a problem hiding this comment.
ensureContainersCreated, called in the @Before ensures the container is created and all data within the container is deleted.
The collectionManager is annotated with @ClassRule
@ClassRule
public static final IntegrationTestCollectionManager collectionManager = new IntegrationTestCollectionManager();
and the internal implementation will ensure the collections are deleted when the test is done (what used to be done in the @AfterClass method.
So, @After is not needed because the data is deleted in the @before (since ensureContainersCreated is also responsible for creating the containers, you're guaranteed it must be invoked prior to inserting anything into the container) and @AfterClass is not needed because that is covered in the @ClassRule
| public static final IntegrationTestCollectionManager collectionManager = new IntegrationTestCollectionManager(); | ||
|
|
||
| @Autowired | ||
| private CosmosTemplate template; |
There was a problem hiding this comment.
Why do we need CosmosTemplate instance for reactive auditable integration test ?
There was a problem hiding this comment.
Because I was being lazy, I'll update the relevant classes to support either CosmosTemplate or ReactiveCosmosTemplate.
There was a problem hiding this comment.
Thanks, that would be wonderful. I would like to see a clear separation of these two templates, if we can. That would enable us to modify either of them, without affecting the other group.
|
@kushagraThapar okay, i think i've addressed all the issues you raised, have a look and let me know. Thanks! |
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
The changes look good, I am running final integration tests, once they succeed I think it is better to merge this PR, and you can have follow up PR for the remaining changes. |
|
@Blackbaud-MikeLueders - Because we are touching |
Blackbaud-MikeLueders
force-pushed
the
rework-tests
branch
from
e9370e1 to
d53f61b
Compare
|
@kushagraThapar I went ahead and converted the remaining tests. The reason those tests were passing at all before was probably luck - I added the AuditableEntity tests to those a while back but I never ensured the containers were created at the start of the test, probably just a matter of test ordering that they worked at all. However, after converting the rest of the tests, I'm realizing my initial strategy of storing the lock in the target container is not going to work. There are some tests that are retrieving all values in their containers and are blowing up b/c the lock is not of the shape they are expecting, which makes sense. I'm going to rework things slightly and create a separate container that holds all the locks. |
Blackbaud-MikeLueders
force-pushed
the
rework-tests
branch
from
b1d3bd7 to
14eafa2
Compare
|
@kushagraThapar Okay, reworked things a bit to use a separate container to hold the locks. Also, since some collections are re-used between tests, I changed it so instead of deleting the collection after a test class finishes, collections are deleted once the complete test run has finished. |
That makes sense, I think it is best to just initialize the collections as we go, but only delete them after the whole test suite is done. |
Blackbaud-MikeLueders
force-pushed
the
rework-tests
branch
3 times, most recently
from
0bda80f to
4dc38c5
Compare
Blackbaud-MikeLueders
force-pushed
the
rework-tests
branch
2 times, most recently
from
0b80e07 to
d1eddcc
Compare
|
@kushagraThapar i triggered the integration tests several times over the weekend by pushing up minor commits and the spring integration tests never failed so this should be good to go |
Yups, I saw those, thanks for triggering those, I think this looks good, I will do a final review today and will get it in. @Blackbaud-MikeLueders - will let you know if I see any issues, but overall, this is very great. This will definitely help with improving our test surface. |
There was a problem hiding this comment.
New changes look good. Will run the live integration tests to verify all the tests runs, and will merge it then. Thanks again @Blackbaud-MikeLueders for this useful change.
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Add a simple container locking mechanism to enable integration tests to be run concurrently without interfering with one another. The lock is acquired during the initial test execution (waiting up to 10 minutes for lease acquisition) and the lease is refreshed on every test execution. The lock has a lease expiration (5 minutes) so if something goes wrong and the process dies while holding the lease, a separate process will eventually be able to acquire it again. This does mean that each test method should not take longer than the lease duration to run, though currently that should not be an issue (and the lease duration can always be adjusted if need be).