DS-4499 Fix cleanup of test environment in earlier Integration Test #2759

Micheleboychuk · 2020-05-11T13:34:49Z

References

https://jira.lyrasis.org/browse/DS-4499

Description

*All IT classes have been reviewed and where the context.restoreAuthSystemState() was missing, it has been added.
*Objects that were not used have been removed.
*Refactoring of the cleanup method in the Builder classes has been done.

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

My PR is small in size (e.g. less than 1,000 lines of code, not including comments & integration tests). Exceptions may be made if previously agreed upon.
My PR passes Checkstyle validation based on the Code Style Guide
My PR includes Javadoc for all new (or modified) public methods and classes. It also includes Javadoc for large or complex private methods.
My PR passes all tests and includes new/updated Unit or Integration Tests for any bug fixes, improvements or new features. A few reminders about what constitutes good tests:
- Include tests for different user types, including: (1) Anonymous user, (2) Logged in user (non-admin), and (3) Administrator.
- Include tests for known error scenarios and error codes (e.g. 400 Bad Request, 401 Unauthorized, 403 Forbidden, 404 Not Found, etc)
- For bug fixes, include a test that reproduces the bug and proves it is fixed. For clarity, it may be useful to provide the test in a separate commit from the bug fix.
N/A If my PR includes new, third-party dependencies (in any pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
N/A If my PR modifies the REST API, I've linked to the REST Contract page (or open PR) related to this change.

…flowItemRestRepositoryIT

…tControllerIT

abollini

I have added some inline comments to facilitate the review by other. @Micheleboychuk are going to resolve the conflict and verify that travis complete successful, once done I will remove the work in progress label

dspace-server-webapp/src/test/java/org/dspace/app/rest/builder/AbstractDSpaceObjectBuilder.java

abollini · 2020-05-12T07:31:16Z

dspace-server-webapp/src/test/java/org/dspace/app/rest/CommunityAdminGroupRestControllerIT.java

@@ -138,6 +138,8 @@ public void postCommunityAdminGroupCreateAdminGroupSuccess() throws Exception {
                        .andDo(result -> idRef
                            .set(UUID.fromString(read(result.getResponse().getContentAsString(), "$.id")))
                        );
+        // no needs to explicitly cleanup the group created as the community comes


for other reviewers please note this comment as it explains the need of the refactoring done in all the Builder about the cleanup process

abollini · 2020-05-12T07:42:49Z

dspace-server-webapp/src/test/java/org/dspace/app/rest/builder/CollectionBuilder.java

-                collectionService.removeAdministrators(c, collection);
+       try (Context c = new Context()) {
+            c.turnOffAuthorisationSystem();
+            collection = c.reloadEntity(collection);


This is a comment for other reviewers to explain the reasoning behind this change (and the same in all the other builder).
It is better to reload the primary object as this will guarantee us to access also fresh related objects during the cleanup, in this case the Admin Group.

Before, all the related objects where reloaded just before to be deleted (see previous implementeation of the deleteWorkflowGroups) but this approach have several issues:

it is easy to forget about it, indeed when the deleteAdminGroup method was added the first time the build was failing due to that

doing that manually each time duplicate lot of code

as you only reload the object that was attached to the "old" copy of the primary object you will fail to find related object created in outside thread. If the admin group has been created via a REST call collection.getAdministrators() run before the refresh of the community object would have returned null

Just a minor comment, I wonder if we should add a small inline comment here to remind ourselves why the reloadEntity is necessary. That way we don't accidentally refactor it out at a later time.

We could just say:
// Ensure object and any related objects are reloaded before checking to see what needs cleanup

abollini · 2020-05-12T13:57:45Z

it is ready to review now. A nice side effect of the refactoring seems to be an increased speedup in the test execution. The server tests took 12minutes less compared to the last build on the master

tdonohue

@Micheleboychuk and @abollini : This looks great overall. I only have a few minor comments inline, and then I'd add a +1. See below.

tdonohue · 2020-05-12T15:58:35Z

dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonRestRepositoryIT.java

@@ -1523,6 +1535,8 @@ public void patchMultipleOperationsWithFailure() throws Exception {
                                        .withEmail("Johndoe@example.com")
                                        .build();

+        context.restoreAuthSystemState();
+
        String token = getAuthToken(admin.getEmail(), password);

        context.restoreAuthSystemState();


It looks like there's accidentally two context.restoreAuthSystemState() calls in this method now. This second one can be removed.

tdonohue · 2020-05-12T16:10:43Z

dspace-server-webapp/src/test/java/org/dspace/app/rest/builder/BundleBuilder.java

@@ -67,4 +76,20 @@ public Bundle build() throws SQLException, AuthorizeException {

        return bundle;
    }
+
+    public static void deleteBundel(UUID uuid) throws SQLException, IOException {


This method is misspelled. It should be deleteBundle. Obviously, you'll need to also cleanup the places it's used in ITs.

tdonohue · 2020-05-12T16:14:56Z

dspace-server-webapp/src/test/java/org/dspace/app/rest/builder/CollectionBuilder.java

-                collectionService.removeAdministrators(c, collection);
+       try (Context c = new Context()) {
+            c.turnOffAuthorisationSystem();
+            collection = c.reloadEntity(collection);


Just a minor comment, I wonder if we should add a small inline comment here to remind ourselves why the reloadEntity is necessary. That way we don't accidentally refactor it out at a later time.

We could just say:
// Ensure object and any related objects are reloaded before checking to see what needs cleanup

tdonohue

👍 This looks good to me now! Thanks @Micheleboychuk and @abollini , as I think these are great improvements...and it's wonderful to hear it also significantly improves the time it takes for the tests to complete.

benbosman

Can you add detail about the changes:

Why is it now required to explicitly delete e.g. items from a builder, this used to be automated
What's the purpose of .andDo(result -> idRef.set(read(result.getResponse().getContentAsString(), "$.id")))
What's the purpose of .andDo(result -> idRefNoEmbeds.set(UUID.fromString(read(result.getResponse().getContentAsString(), "$.id"))))

dspace-server-webapp/src/test/java/org/dspace/app/rest/builder/BitstreamBuilder.java

dspace-server-webapp/src/test/java/org/dspace/app/rest/builder/BundleBuilder.java

abollini · 2020-05-20T14:36:56Z

hi @benbosman
thanks for reviewing that. I can answer your first question

Why is it now required to explicitly delete e.g. items from a builder, this used to be automated

this is not required, no changes in such direction. If we have added an explicit deletion of an item created with an item builder this is most probably superfluous. Can you point to the specific line so that I can eventually double check?

What's the purpose of .andDo(result -> idRef.set(read(result.getResponse().getContentAsString(), "$.id")))
What's the purpose of .andDo(result -> idRefNoEmbeds.set(UUID.fromString(read(result.getResponse().getContentAsString(), "$.id"))))

in general they are used to grab the ID of object created via REST request so that they can be cleaned later. Do you have any specific lines that I need to double check?

benbosman

Thanks @abollini for the feedback. This looks good to me
I haven't tested it, but since it only affects ITs and Travis succeeds, it shouldn't be a problem

Micheleboychuk added 12 commits May 6, 2020 13:43

added context.restoreAuthSystemState(), where was missing

1c4a43c

fixed error

c21ef22

added context.restoreAuthSystemState() and delete static methods

8c9e809

removed unused dipendensies

a975daa

refactored RelationshipRestRepositoryIT

3b22efd

cleanup of test environment

f91724d

cleanup of test environment in WorkspaceItemRestRepositoryIT and Work…

a2e6175

…flowItemRestRepositoryIT

cleanup of CollectionRestRepositoryIT and ItemRestRepositoryIT

95d089a

cleanup of CollectionGroupRestControllerIT and CommunityAdminGroupRes…

47411d8

…tControllerIT

added deleteAdminGroup method for cleanup in CommunityBuilder

6c24ff8

refactoring of some builder classes

7fc6f6c

refactoring of the cleanup method in Builder classes

f0ec92d

abollini reviewed May 12, 2020

View reviewed changes

abollini added interface: REST API v7+ REST API for v7 and later (dspace-server-webapp module) work in progress PR is still being worked on & is not currently ready for review labels May 12, 2020

Micheleboychuk and others added 2 commits May 12, 2020 10:04

removed a note

8c5ff93

Merge master into DS-4499-CleanupTestEnviroment

02a9a3f

abollini removed the work in progress PR is still being worked on & is not currently ready for review label May 12, 2020

tdonohue reviewed May 12, 2020

View reviewed changes

Micheleboychuk added 2 commits May 13, 2020 18:36

Implement community feedbacks

4ab8605

Implement community feedbacks

d462aa7

tdonohue approved these changes May 14, 2020

View reviewed changes

benbosman reviewed May 20, 2020

View reviewed changes

dspace-server-webapp/src/test/java/org/dspace/app/rest/builder/BitstreamBuilder.java Show resolved Hide resolved

dspace-server-webapp/src/test/java/org/dspace/app/rest/builder/BundleBuilder.java Show resolved Hide resolved

benbosman approved these changes May 20, 2020

View reviewed changes

tdonohue merged commit 03de94e into DSpace:master May 20, 2020

tdonohue mentioned this pull request May 21, 2020

DS-4027: New user registration & forgot password #2763

Merged

6 tasks

tdonohue mentioned this pull request May 29, 2020

Scripts & processes: importing and exporting csv's #2648

Merged

tdonohue added this to Needs Reviewers Assigned in DSpace 7 Beta 3 via automation Jun 30, 2020

tdonohue moved this from Needs Reviewers Assigned to Done in DSpace 7 Beta 3 Jun 30, 2020

tdonohue added this to the 7.0beta3 milestone Jun 30, 2020

Micheleboychuk deleted the DS-4499-CleanupTestEnviroment branch February 22, 2022 22:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DS-4499 Fix cleanup of test environment in earlier Integration Test #2759

DS-4499 Fix cleanup of test environment in earlier Integration Test #2759

Micheleboychuk commented May 11, 2020 •

edited

abollini left a comment

abollini May 12, 2020

abollini May 12, 2020

tdonohue May 12, 2020

abollini commented May 12, 2020

tdonohue left a comment

tdonohue May 12, 2020

tdonohue May 12, 2020

tdonohue May 12, 2020

tdonohue left a comment

benbosman left a comment

abollini commented May 20, 2020

benbosman left a comment

DS-4499 Fix cleanup of test environment in earlier Integration Test #2759

DS-4499 Fix cleanup of test environment in earlier Integration Test #2759

Conversation

Micheleboychuk commented May 11, 2020 • edited

References

Description

Checklist

abollini left a comment

Choose a reason for hiding this comment

abollini May 12, 2020

Choose a reason for hiding this comment

abollini May 12, 2020

Choose a reason for hiding this comment

tdonohue May 12, 2020

Choose a reason for hiding this comment

abollini commented May 12, 2020

tdonohue left a comment

Choose a reason for hiding this comment

tdonohue May 12, 2020

Choose a reason for hiding this comment

tdonohue May 12, 2020

Choose a reason for hiding this comment

tdonohue May 12, 2020

Choose a reason for hiding this comment

tdonohue left a comment

Choose a reason for hiding this comment

benbosman left a comment

Choose a reason for hiding this comment

abollini commented May 20, 2020

benbosman left a comment

Choose a reason for hiding this comment

Micheleboychuk commented May 11, 2020 •

edited