Skip to content
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.

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

[DS-3594] Refine unit tests to run against postgres #1746

Merged
merged 5 commits into from May 31, 2017
Merged
Changes from 4 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -97,7 +97,7 @@ public void testDeleteByDateAndCode()
qry.setInteger("id", checkId);
qry.setDate("date", matchDate);
qry.setString("result", ChecksumResultCode.CHECKSUM_MATCH.name());
qry.setString("bitstream", bs.getID().toString()); // FIXME identifier not being set???
qry.setParameter("bitstream", bs.getID()); // FIXME identifier not being set???
qry.executeUpdate();

// Row with nonmatching result code
@@ -107,7 +107,7 @@ public void testDeleteByDateAndCode()
qry.setInteger("id", checkId);
qry.setDate("date", noMatchDate);
qry.setString("result", ChecksumResultCode.CHECKSUM_NO_MATCH.name());
qry.setString("bitstream", bs.getID().toString());
qry.setParameter("bitstream", bs.getID()); // FIXME identifier not being set???
qry.executeUpdate();

// Create one newer row
@@ -117,7 +117,7 @@ public void testDeleteByDateAndCode()
qry.setInteger("id", checkId);
qry.setDate("date", new java.sql.Date(futureDate.getTime()));
qry.setString("result", ChecksumResultCode.CHECKSUM_MATCH.name());
qry.setString("bitstream", bs.getID().toString());
qry.setParameter("bitstream", bs.getID()); // FIXME identifier not being set???
qry.executeUpdate();

// Test!
@@ -122,7 +122,7 @@ public void testInstallItem_Context_InProgressSubmission() throws Exception
public void testInstallItem_validHandle() throws Exception
{
context.turnOffAuthorisationSystem();
String handle = "123456789/567";
String handle = "123456789/56789";
WorkspaceItem is = workspaceItemService.create(context, collection, false);

//Test assigning a specified handle to an item
@@ -149,7 +149,7 @@ public void testInstallItem_invalidHandle() throws Exception
authorizeService.isAdmin((Context) any); result = true;
}};

String handle = "123456789/567";
String handle = "123456789/56789";
WorkspaceItem is = workspaceItemService.create(context, collection, false);
WorkspaceItem is2 = workspaceItemService.create(context, collection, false);

@@ -170,7 +170,7 @@ public void testInstallItem_invalidHandle() throws Exception
public void testRestoreItem() throws Exception
{
context.turnOffAuthorisationSystem();
String handle = "123456789/567";
String handle = "123456789/56789";
WorkspaceItem is = workspaceItemService.create(context, collection, false);

//get current date
@@ -240,7 +240,7 @@ public void testInstallItem_todayAsIssuedDate() throws Exception
{
//create a dummy WorkspaceItem
context.turnOffAuthorisationSystem();
String handle = "123456789/567";
String handle = "123456789/56789";
WorkspaceItem is = workspaceItemService.create(context, collection, false);

// Set "today" as "dc.date.issued"
@@ -274,7 +274,7 @@ public void testInstallItem_nullIssuedDate() throws Exception
{
//create a dummy WorkspaceItem with no dc.date.issued
context.turnOffAuthorisationSystem();
String handle = "123456789/567";
String handle = "123456789/56789";
WorkspaceItem is = workspaceItemService.create(context, collection, false);

Item result = installItemService.installItem(context, is, handle);
@@ -293,7 +293,7 @@ public void testRestoreItem_todayAsIssuedDate() throws Exception
{
//create a dummy WorkspaceItem
context.turnOffAuthorisationSystem();
String handle = "123456789/567";
String handle = "123456789/56789";
WorkspaceItem is = workspaceItemService.create(context, collection, false);

// Set "today" as "dc.date.issued"
@@ -107,28 +107,30 @@ public void init()
@Override
public void destroy()
{
// try {
context.turnOffAuthorisationSystem();
//Get new instances, god knows what happended before
// it = itemService.find(context, it.getID());
// collection = collectionService.find(context, collection.getID());
// owningCommunity = communityService.find(context, owningCommunity.getID());
//
// communityService.delete(context, owningCommunity);
// context.commit();
// context.restoreAuthSystemState();
it = null;
collection = null;
owningCommunity = null;
context.turnOffAuthorisationSystem();
try {
itemService.delete(context, it);
} catch(Exception e){
}

try {
collectionService.delete(context, collection);
} catch(Exception e){
}

try {
communityService.delete(context, owningCommunity);
} catch(Exception e){
}

context.restoreAuthSystemState();
it = null;
collection = null;
owningCommunity = null;
try {
super.destroy();
// } catch (SQLException | AuthorizeException | IOException ex) {
// if(context.isValid())
// {
// context.abort();
// }
// log.error("Error in destroy", ex);
// fail("Error in destroy: " + ex.getMessage());
// }
} catch(Exception e){
}
}


@@ -206,6 +206,9 @@ public void findAllNameSort() throws SQLException {
List<String> names = new ArrayList<>();
List<String> sortedNames = new ArrayList<>();
for (Group group : groups) {
if (group.getName() == null) {
continue;

This comment has been minimized.

Copy link
@pnbecker

pnbecker May 16, 2017

Member

I have in mind, that I run in trouble when using an unnamed group in a unit test once. If I remember correctly, somewhere in our AuthorizeService or GroupService we still use group names instead of UUIDs. I think it would be appropriate to either through an error here. Maybe we should create another jira iisue about group without names?

This comment has been minimized.

Copy link
@tdonohue

tdonohue May 16, 2017

Member

I also do wonder about this change. If groups are getting into the system with empty names, then our tests should be able to handle it. Does this change end up hiding a larger issue?

This comment has been minimized.

Copy link
@mwoodiupui

mwoodiupui May 16, 2017

Member

Why would we want to support un-named groups? Shouldn't it be made impossible to persist a group with no name?

This comment has been minimized.

Copy link
@tdonohue

tdonohue May 16, 2017

Member

@mwoodiupui : we agree, but are coming at it from different directions. Either groups should not be getting in without a name (this is the ideal situation), or we should have a way to handle unnamed groups. Either way, I worry this unit test modification could be hiding a problem...why/how are unnamed groups getting in there? Maybe there's a separate Unit Test that is creating unnamed groups (and shouldn't be doing so)?

This comment has been minimized.

Copy link
@tdonohue

tdonohue May 16, 2017

Member

After a deeper analysis, it looks like we have several other tests that create unnamed Groups. Here's one: https://github.com/Georgetown-University-Libraries/DSpace/blob/ebf256caa1f03194cb547dae7eada64428a1b3e2/dspace-api/src/test/java/org/dspace/eperson/GroupTest.java#L125

So, the issue here seems to be that in a persistent DB (like Postgres), these unnamed groups persist. We likely should be cleaning them up after their test runs/succeeds.

This comment has been minimized.

Copy link
@terrywbrady

terrywbrady May 16, 2017

Author Contributor

If I run the existing set of tests against postgres, there are dozens of groups, epersons, items, and collections that persist. I am finding it particularly challenging to delete EPersons and Groups in the new test that I am writing. For the new tests, I am assuming that I will need to wipe out the database between runs. If we want the tests to clean up after themselves, that should probably be captured in a new (more complex) ticket.

This comment has been minimized.

Copy link
@terrywbrady

terrywbrady May 18, 2017

Author Contributor

@tdonohue , @mwoodiupui , are you requesting changes before this can be merged?

This comment has been minimized.

Copy link
@tdonohue

tdonohue May 19, 2017

Member

@terrywbrady : I think minimally this needs a comment to its purpose. It's really odd looking in a test, as it implies that Groups can exist without a name (and the test now supports that concept, when it probably shouldn't). My original comment was stating that we may want to look closer at the other tests in this class to see which ones are creating groups that have no name, as they may be more "at fault" here. If we can fix the other tests, then this check for null names should be unnecessary.

As an example the createGroup() test here creates an unnamed Group. But, it does a good job of cleaning up that unnamed group in a finally section: https://github.com/Georgetown-University-Libraries/DSpace/blob/ebf256caa1f03194cb547dae7eada64428a1b3e2/dspace-api/src/test/java/org/dspace/eperson/GroupTest.java#L121 I'm wondering if we have one or more other test cases here that are creating unnamed groups but not cleaning them up in a finally section.... as those may be the root cause of your issue here.

This comment has been minimized.

Copy link
@tdonohue

tdonohue May 19, 2017

Member

One problem may actually be in the init() section which creates a bunch of unnamed groups (these likely should all have dummy names to begin with). https://github.com/Georgetown-University-Libraries/DSpace/blob/ebf256caa1f03194cb547dae7eada64428a1b3e2/dspace-api/src/test/java/org/dspace/eperson/GroupTest.java#L56

There may still be other methods creating unnamed groups as well.

This comment has been minimized.

Copy link
@terrywbrady

terrywbrady May 22, 2017

Author Contributor

@tdonohue , I took a closer look at the usage of the GroupService.create(Context) method. This method is called.
Outside of unit test files, setName() seems to be called consistently after the create method.

Would it make sense to set a default string like "New Group" within the create(Context) method to provide a default value in the case where the sort is executed?

This comment has been minimized.

Copy link
@tdonohue

tdonohue May 23, 2017

Member

@terrywbrady : thanks for the update. This may be a bit of a lost cause here...it's possible that a separate Test file is adding these unnamed groups. Let's just go with this code as-is, as it's not worth trying to find the needle in the haystack here. But, let's add in a comment as to why the if statement is here. Something like:

// Ignore any unnamed groups. This is only necessary when running unit tests via a persistent database (e.g. Postgres) as unnamed groups may be created by other tests.

This comment has been minimized.

Copy link
@terrywbrady

terrywbrady May 23, 2017

Author Contributor

Thanks @tdonohue . Comment added.

}
names.add(group.getName());
sortedNames.add(group.getName());
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.