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

Conversation

Projects
None yet
4 participants
@terrywbrady
Copy link
Contributor

commented May 15, 2017

@pnbecker
Copy link
Member

left a comment

I reviewed the code changes. Beside a minor node about how to deal with groups without names, the changes looks reasonable to me. I haven't tested it.

@@ -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
@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.

@terrywbrady terrywbrady merged commit 094f775 into DSpace:dspace-6_x May 31, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@terrywbrady terrywbrady deleted the Georgetown-University-Libraries:ds3594 branch May 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.