JCL-419: harden e2e code #576
Conversation
integration/base/src/main/java/com/inrupt/client/integration/base/AccessGrantScenarios.java
Outdated
Show resolved
Hide resolved
| createContainer(publicContainerURI); | ||
| prepareACR(publicContainerURI); | ||
| //if a tests fails it can be that the cleanup was not properly done, so we do it here too | ||
| Utils.deleteContentsRecursively(localAuthClient, publicContainerURI); |
There was a problem hiding this comment.
As mentioned elsewhere, we can't delete this data here, because it will interfere with other concurrently-running tests.
integration/base/src/main/java/com/inrupt/client/integration/base/AuthenticationScenarios.java
Outdated
Show resolved
Hide resolved
| privateContainerURI = URIBuilder.newBuilder(URI.create(podUrl)) | ||
| .path(PRIVATE_RESOURCE_PATH + "/").build(); | ||
| //if a tests fails it can be that the cleanup was not properly done, so we do it here too | ||
| Utils.deleteContentsRecursively(localAuthClient, privateContainerURI); |
There was a problem hiding this comment.
We don't want to delete data in any setup phase, as it may interfere with other tests.
integration/base/src/main/java/com/inrupt/client/integration/base/CoreModulesResource.java
Outdated
Show resolved
Hide resolved
| createContainer(publicContainerURI); | ||
| prepareACR(publicContainerURI); | ||
| //if a tests fails it can be that the cleanup was not properly done, so we do it here too | ||
| Utils.deleteContentsRecursively(localAuthClient, publicContainerURI); |
integration/base/src/main/java/com/inrupt/client/integration/base/Utils.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| static void deleteRecursive(final SolidSyncClient client, final URI url, final AtomicInteger depth) { |
There was a problem hiding this comment.
I would avoid the use of a shared AtomicInteger here; instead, you can just have a final int depth and perform arithmetic operations on that in the recursive calls
There was a problem hiding this comment.
Yes, the AtomicInteger was useful in an asynchronous context
There was a problem hiding this comment.
However, I cannot use it as final int depth but I do need a final in the lambda later. I did not find a better solution.
edwardsph
left a comment
edwardsph
left a comment
There was a problem hiding this comment.
Taking a step back and looking at how you manage test containers, this is what I see:
If PUBLIC_RESOURCE_PATH doesn't exist then you have
publicTestContainerURI = podUrl/test-UUID/
and you rely on the ACR of the pod root being right.
If it does exist, then you have
publicTestContainerURI = podUrl/PUBLIC_RESOURCE_PATH/test-UUID/
Then you create podUrl/PUBLIC_RESOURCE_PATH/ and set up the ACR and either tear down this container at the end or try and ensure it is empty at the start. Since this is a fixed location, it will clash with other test runs.
Why not make that container unique too so you create podUrl/PUBLIC_RESOURCE_PATH-UUID/. That would isolate your tests but still use a defined prefix to make the test container identifiable (if that was the reason for allowing someone to set PUBLIC_RESOURCE_PATH. I may not have understood why you want someone to define the test container outside the tests.
The downside of my approach is that if something goes wrong with teardown, you can be left with old test containers but they will not interfere with anything and you can have a separate housekeeping process to clean them out. However, I think you will find that once the tests are more stable you will have less of a problem with this.
integration/base/src/main/java/com/inrupt/client/integration/base/CoreModulesResource.java
Outdated
Show resolved
Hide resolved
| final URI testRDFresourceURI = URIBuilder.newBuilder(privateContainerURI) | ||
| .path("resource-accessGrantGetRdfTest.ttl") | ||
| .build(); | ||
|
|
||
| try (final SolidRDFSource resource = new SolidRDFSource(testRDFresourceURI, null, null)) { | ||
| assertDoesNotThrow(() -> resourceOwnerClient.create(resource)); |
There was a problem hiding this comment.
A pattern I typically use with the JS client for such case, where the resource URI is irrelevant to the client (but still needs to be tracked) is to get the server to create the resource by POSTing to the parent container, which URL the client does care about (here that would be privateContainerURI). Is this a pattern that would be possible with the JCL, or does the target URL needs to be known before creating a resource?
There was a problem hiding this comment.
In the high-level client, we exclusively use PUT for create and update. POST and PATCH are usable directly in the low-level client.
There was a problem hiding this comment.
It's absolutely not critical for GA, but would something like createChild make sense in the SolidClient ?
We saw lately too many CI problems. On investigation, it turned out that the problem occurs because the tests do not do a proper cleanup.
A solution here is to also clean up before starting the tests (making sure the containers are empty for example).
It still happens that some tests randomly fail, I am not sure if we can simply up the retry number...