Skip to content

Commit

Permalink
SAK-32283 Remove tight binding sitestats -> kernel (sakaiproject#4029)
Browse files Browse the repository at this point in the history 
* SAK-32283 Remove tight binding sitestats -> kernel

Sitestats implements lots of kernel APIs for testing, this means that
when a kernel API changes the test classes in sitestats need updated. In
almost all cases the change is just to include an automatically
generated stub method. Instead we generate these stub methods using
mockito for unit tests and bytebuddy for integration tests.

It also makes the mock classes much clearer as you can clearly see what
is needed for the tests to pass, rather than being blinded by the
autogenerated stub methods.

Mockito isn't designed for multithreaded integrations tests and
re-writing the tests to use mockito throughout is lots more work so this
is a interim solution.

I also fixed up one of the performance tests so it runs against hsqldb
but doesn't practically test anything. The reason is so that the code
doesn't rot, otherwise if it's not compiled/run it's easy to break in a
refactoring or library upgrade when the fix is trivial as you're doing
lots of other similar fixes.

* Fixing typo.
  • Loading branch information
@buckett @jonespm
buckett authored and jonespm committed Mar 8, 2017
1 parent 1fcc4a2 commit 708b4d5
Show file tree
Hide file tree
Showing 39 changed files with 294 additions and 4,371 deletions.
2 changes: 1 addition & 1 deletion sitestats/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
<!-- Project versions -->
<properties>
<commons-betwixt.version>0.8</commons-betwixt.version>
<easymock.version>3.2</easymock.version>
<easymock.version>3.4</easymock.version>
<h2.version>1.1.111</h2.version>
<mysql.version>5.1.25</mysql.version>
<!-- <ojdbc.version>10.2.0.2.0</ojdbc.version> -->
Expand Down
16 changes: 16 additions & 0 deletions sitestats/sitestats-impl/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,12 @@
<artifactId>servlet-api</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>net.bytebuddy</groupId>
<artifactId>byte-buddy</artifactId>
<version>1.5.3</version>
<scope>test</scope>
</dependency>
</dependencies>
<build>

Expand All @@ -232,6 +238,16 @@
<directory>src/bundle</directory>
</resource>
</resources>
<testResources>
<testResource>
<directory>src/test</directory>
<includes>
<include>**/*.xml</include>
<include>**/*.properties</include>
<include>**/*.sql</include>
</includes>
</testResource>
</testResources>
</build>

<profiles>
Expand Down
48 changes: 34 additions & 14 deletions sitestats/sitestats-impl/src/test/fake-services.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,41 @@
xsi:schemaLocation="http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd">

<!-- FAKE Services -->
<bean id="org.sakaiproject.mock.service.SiteService" class="org.mockito.Mockito" factory-method="mock">
<constructor-arg value="org.sakaiproject.site.api.SiteService" />
<bean id="org.sakaiproject.sitestats.test.mocks.FakeToolManager" class="org.mockito.Mockito" factory-method="spy">
<constructor-arg value="org.sakaiproject.sitestats.test.mocks.FakeToolManager" type="java.lang.Class"/>
</bean>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeEventRegistryService" class="org.mockito.Mockito" factory-method="spy">
<constructor-arg value="org.sakaiproject.sitestats.test.mocks.FakeEventRegistryService" type="java.lang.Class"/>
</bean>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeAliasService" class="org.mockito.Mockito" factory-method="spy">
<constructor-arg value="org.sakaiproject.sitestats.test.mocks.FakeAliasService" type="java.lang.Class"/>
</bean>
<bean id="org.sakaiproject.mock.service.UserDirectoryService" class="org.mockito.Mockito" factory-method="mock">
<constructor-arg value="org.sakaiproject.user.api.UserDirectoryService" />
<bean id="org.sakaiproject.sitestats.test.mocks.FakeEntityManager" class="org.mockito.Mockito" factory-method="spy">
<constructor-arg value="org.sakaiproject.sitestats.test.mocks.FakeEntityManager" type="java.lang.Class"/>
</bean>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeEventTrackingService" class="org.mockito.Mockito" factory-method="spy">
<constructor-arg value="org.sakaiproject.sitestats.test.mocks.FakeEventTrackingService" type="java.lang.Class"/>
</bean>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeServerConfigurationService" class="org.mockito.Mockito" factory-method="spy">
<constructor-arg value="org.sakaiproject.sitestats.test.mocks.FakeServerConfigurationService" type="java.lang.Class"/>
</bean>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeToolManager" class="org.sakaiproject.sitestats.test.mocks.FakeToolManager"/>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeEventRegistryService" class="org.sakaiproject.sitestats.test.mocks.FakeEventRegistryService"/>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeAliasService" class="org.sakaiproject.sitestats.test.mocks.FakeAliasService"/>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeEntityManager" class="org.sakaiproject.sitestats.test.mocks.FakeEntityManager"/>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeEventTrackingService" class="org.sakaiproject.sitestats.test.mocks.FakeEventTrackingService"/>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeUsageSessionService" class="org.sakaiproject.sitestats.test.mocks.FakeUsageSessionService"/>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeServerConfigurationService" class="org.sakaiproject.sitestats.test.mocks.FakeServerConfigurationService"/>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeMemoryService" class="org.sakaiproject.memory.mock.MemoryService"/>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeSessionManager" class="org.sakaiproject.sitestats.test.mocks.FakeSessionManager"/>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeStatsAuthz" class="org.sakaiproject.sitestats.test.mocks.FakeStatsAuthz"/>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeUserDirectoryService" class="org.sakaiproject.sitestats.test.mocks.FakeUserDirectoryService"/>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeSessionManager" class="org.mockito.Mockito" factory-method="spy">
<constructor-arg value="org.sakaiproject.sitestats.test.mocks.FakeSessionManager" type="java.lang.Class"/>
</bean>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeUserDirectoryService" class="org.mockito.Mockito" factory-method="spy">
<constructor-arg value="org.sakaiproject.sitestats.test.mocks.FakeUserDirectoryService" type="java.lang.Class"/>
</bean>

<bean id="org.sakaiproject.sitestats.test.mocks.FakeStatsAuthz" class="org.mockito.Mockito" factory-method="mock">
<constructor-arg value="org.sakaiproject.sitestats.api.StatsAuthz"/>
</bean>
<bean id="org.sakaiproject.sitestats.test.mocks.FakeUsageSessionService" class="org.mockito.Mockito" factory-method="mock">
<constructor-arg value="org.sakaiproject.event.api.UsageSessionService"/>
</bean>
<bean id="org.sakaiproject.mock.service.SiteService" class="org.mockito.Mockito" factory-method="mock">
<constructor-arg value="org.sakaiproject.site.api.SiteService" />
</bean>

<alias name="org.sakaiproject.sitestats.test.mocks.FakeUserDirectoryService" alias="org.sakaiproject.mock.service.UserDirectoryService"/>
</beans>
11 changes: 8 additions & 3 deletions ...tats/sitestats-impl/src/test/org/sakaiproject/sitestats/test/EventAggregatorTestPerf.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import java.util.List;
import java.util.Set;

import org.mockito.Mockito;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.easymock.IAnswer;
Expand All @@ -51,6 +52,7 @@
import org.sakaiproject.sitestats.test.mocks.FakeSite;
import org.sakaiproject.time.api.Time;
import org.sakaiproject.util.ResourceLoader;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit4.AbstractJUnit4SpringContextTests;

Expand Down Expand Up @@ -81,12 +83,15 @@ public class EventAggregatorTestPerf extends AbstractJUnit4SpringContextTests {
private List<String> siteUsers;
private List<String> siteResources;

@Autowired
public void setStatsManager(StatsManager M_sm) {
this.M_sm = M_sm;
}
@Autowired
public void setStatsUpdateManager(StatsUpdateManager M_sum) {
this.M_sum = M_sum;
}
@Autowired
public void setEventTrackingService(EventTrackingService M_ets) {
this.M_ets = M_ets;
}
Expand All @@ -95,7 +100,7 @@ public void setEventTrackingService(EventTrackingService M_ets) {
public void onSetUp() throws Exception {
/** run this before each test starts */
LOG.debug("Setting up tests...");

// Setup site users
siteUsers = new ArrayList<String>();
for(int i=0; i<MAX_USERS; i++) {
Expand All @@ -110,7 +115,7 @@ public void onSetUp() throws Exception {
// Site A:
// - tools {SiteStats, Chat, Resources}
// - users {user-0, user-1, ... , user-999}
Site siteA = new FakeSite(siteId,
Site siteA = Mockito.spy(FakeSite.class).set(siteId,
Arrays.asList(StatsManager.SITESTATS_TOOLID, FakeData.TOOL_CHAT, StatsManager.RESOURCES_TOOLID)
);
Set<String> usersSet = new HashSet<String>(siteUsers);
Expand All @@ -119,7 +124,7 @@ public void onSetUp() throws Exception {
expect(M_ss.getSite(siteId)).andStubReturn(siteA);
expect(M_ss.isUserSite(siteId)).andStubReturn(false);
expect(M_ss.isSpecialSite(siteId)).andStubReturn(false);
expect(siteA.getCreatedTime()).andStubReturn((Time)anyObject());
Mockito.when(siteA.getCreatedTime()).thenReturn(Mockito.any(Time.class));
// Site 'non_existent_site' doesn't exist
expect(M_ss.isUserSite("non_existent_site")).andStubReturn(false);
expect(M_ss.isSpecialSite("non_existent_site")).andStubReturn(false);
Expand Down
16 changes: 11 additions & 5 deletions sitestats/sitestats-impl/src/test/org/sakaiproject/sitestats/test/ReportManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,15 @@
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
import org.mockito.Mockito;
import org.sakaiproject.content.api.ContentHostingService;
import org.sakaiproject.event.api.Event;
import org.sakaiproject.exception.IdUnusedException;
import org.sakaiproject.javax.PagingPosition;
import org.sakaiproject.site.api.Site;
import org.sakaiproject.site.api.SiteService;
import org.sakaiproject.sitestats.api.SiteVisits;
import org.sakaiproject.sitestats.api.StatsAuthz;
import org.sakaiproject.sitestats.api.StatsManager;
import org.sakaiproject.sitestats.api.StatsUpdateManager;
import org.sakaiproject.sitestats.api.report.Report;
Expand All @@ -56,6 +58,7 @@
import org.sakaiproject.sitestats.test.mocks.FakeEventRegistryService;
import org.sakaiproject.sitestats.test.mocks.FakeServerConfigurationService;
import org.sakaiproject.sitestats.test.mocks.FakeSite;
import org.sakaiproject.sitestats.test.perf.mock.StubUtils;
import org.sakaiproject.tool.api.ToolManager;
import org.sakaiproject.util.ResourceLoader;
import org.springframework.beans.factory.annotation.Autowired;
Expand All @@ -74,6 +77,8 @@ public class ReportManagerTest extends AbstractTransactionalJUnit4SpringContextT
@Autowired
private StatsUpdateManager M_sum;
@Autowired
private StatsAuthz M_sa;
@Autowired
private DB db;
private SiteService M_ss;
@Autowired
Expand All @@ -83,7 +88,7 @@ public class ReportManagerTest extends AbstractTransactionalJUnit4SpringContextT
@Autowired
private FakeServerConfigurationService M_scs;
private ContentHostingService M_chs;

@Before
public void onSetUp() throws Exception {
db.deleteAll();
Expand All @@ -105,16 +110,16 @@ public void onSetUp() throws Exception {
expect(M_ss.getSite("non_existent_site")).andThrow(new IdUnusedException("non_existent_site")).anyTimes();

// My Workspace - user sites
FakeSite userSiteA = new FakeSite("~"+FakeData.USER_A_ID);
FakeSite userSiteB = new FakeSite("~"+FakeData.USER_B_ID);
FakeSite userSiteA = Mockito.spy(FakeSite.class).set("~"+FakeData.USER_A_ID);
FakeSite userSiteB = Mockito.spy(FakeSite.class).set("~"+FakeData.USER_B_ID);
expect(M_ss.getSiteUserId(FakeData.USER_A_ID)).andStubReturn("~"+FakeData.USER_A_ID);
expect(M_ss.getSiteUserId(FakeData.USER_B_ID)).andStubReturn("~"+FakeData.USER_B_ID);
expect(M_ss.getSiteUserId("no_user")).andStubReturn(null);
expect(M_ss.getSite("~"+FakeData.USER_A_ID)).andStubReturn(userSiteA);
expect(M_ss.getSite("~"+FakeData.USER_B_ID)).andStubReturn(userSiteB);

// Site A has tools {SiteStats, Chat, Resources}, has {user-a,user-b}, created 1 month ago
Site siteA = new FakeSite(FakeData.SITE_A_ID,
Site siteA = Mockito.spy(FakeSite.class).set(FakeData.SITE_A_ID,
Arrays.asList(StatsManager.SITESTATS_TOOLID, FakeData.TOOL_CHAT, StatsManager.RESOURCES_TOOLID)
);
((FakeSite)siteA).setUsers(new HashSet<String>(Arrays.asList(FakeData.USER_A_ID,FakeData.USER_B_ID)));
Expand All @@ -124,7 +129,7 @@ public void onSetUp() throws Exception {
expect(M_ss.isSpecialSite(FakeData.SITE_A_ID)).andStubReturn(false);

// Site B has tools {TOOL_CHAT}, has {user-a}, created 2 months ago
FakeSite siteB = new FakeSite(FakeData.SITE_B_ID, FakeData.TOOL_CHAT);
FakeSite siteB = Mockito.spy(FakeSite.class).set(FakeData.SITE_B_ID, FakeData.TOOL_CHAT);
((FakeSite)siteB).setUsers(new HashSet<String>(Arrays.asList(FakeData.USER_A_ID)));
((FakeSite)siteB).setMembers(new HashSet<String>(Arrays.asList(FakeData.USER_A_ID)));
expect(M_ss.getSite(FakeData.SITE_B_ID)).andStubReturn(siteB);
Expand Down Expand Up @@ -737,6 +742,7 @@ public void testLoadSaveReports() {
rd3.setTitle("Title 3");
rd3.setHidden(false);
rd3.setReportParams(new ReportParams());
Mockito.when(M_sa.isSiteStatsAdminPage()).thenReturn(true);
Assert.assertTrue(M_rm.saveReportDefinition(rd3));

List<ReportDef> list = M_rm.getReportDefinitions(null, true, true);
Expand Down
11 changes: 6 additions & 5 deletions sitestats/sitestats-impl/src/test/org/sakaiproject/sitestats/test/StatsManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
import org.mockito.Mockito;
import org.sakaiproject.content.api.ContentHostingService;
import org.sakaiproject.content.api.ContentTypeImageService;
import org.sakaiproject.event.api.Event;
Expand Down Expand Up @@ -111,16 +112,16 @@ public void onSetUp() throws Exception {
expect(M_ss.getSite("non_existent_site")).andThrow(new IdUnusedException("non_existent_site")).anyTimes();

// My Workspace - user sites
FakeSite userSiteA = new FakeSite("~"+FakeData.USER_A_ID);
FakeSite userSiteB = new FakeSite("~"+FakeData.USER_B_ID);
FakeSite userSiteA = Mockito.spy(FakeSite.class).set("~"+FakeData.USER_A_ID);
FakeSite userSiteB = Mockito.spy(FakeSite.class).set("~"+FakeData.USER_B_ID);
expect(M_ss.getSiteUserId(FakeData.USER_A_ID)).andStubReturn("~"+FakeData.USER_A_ID);
expect(M_ss.getSiteUserId(FakeData.USER_B_ID)).andStubReturn("~"+FakeData.USER_B_ID);
expect(M_ss.getSiteUserId("no_user")).andStubReturn(null);
expect(M_ss.getSite("~"+FakeData.USER_A_ID)).andStubReturn(userSiteA);
expect(M_ss.getSite("~"+FakeData.USER_B_ID)).andStubReturn(userSiteB);

// Site A has tools {SiteStats, Chat}, has {user-a,user-b}, created 1 month ago
Site siteA = new FakeSite(FakeData.SITE_A_ID,
Site siteA = Mockito.spy(FakeSite.class).set(FakeData.SITE_A_ID,
Arrays.asList(StatsManager.SITESTATS_TOOLID, FakeData.TOOL_CHAT, StatsManager.RESOURCES_TOOLID)
);
((FakeSite)siteA).setUsers(new HashSet<String>(Arrays.asList(FakeData.USER_A_ID,FakeData.USER_B_ID)));
Expand All @@ -130,7 +131,7 @@ public void onSetUp() throws Exception {
expect(M_ss.isSpecialSite(FakeData.SITE_A_ID)).andStubReturn(false);

// Site B has tools {TOOL_CHAT}, has {user-a}, created 2 months ago
FakeSite siteB = new FakeSite(FakeData.SITE_B_ID, FakeData.TOOL_CHAT);
FakeSite siteB = Mockito.spy(FakeSite.class).set(FakeData.SITE_B_ID, FakeData.TOOL_CHAT);
((FakeSite)siteB).setUsers(new HashSet<String>(Arrays.asList(FakeData.USER_A_ID)));
((FakeSite)siteB).setMembers(new HashSet<String>(Arrays.asList(FakeData.USER_A_ID)));
expect(M_ss.getSite(FakeData.SITE_B_ID)).andStubReturn(siteB);
Expand All @@ -139,7 +140,7 @@ public void onSetUp() throws Exception {

if(enableLargeMembershipTest) {
// Site C has tools {SiteStats, Chat}, has 2002 users (user-1..user-2002), created 1 month ago
Site siteC = new FakeSite(FakeData.SITE_C_ID,
Site siteC = Mockito.spy(FakeSite.class).set(FakeData.SITE_C_ID,
Arrays.asList(StatsManager.SITESTATS_TOOLID, FakeData.TOOL_CHAT, StatsManager.RESOURCES_TOOLID)
);
List<String> siteCUsersList = new ArrayList<String>();
Expand Down
7 changes: 4 additions & 3 deletions ...stats/sitestats-impl/src/test/org/sakaiproject/sitestats/test/StatsUpdateManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import org.junit.Before;
import org.junit.FixMethodOrder;
import org.junit.Test;
import org.mockito.Mockito;
import org.sakaiproject.event.api.Event;
import org.sakaiproject.event.api.EventTrackingService;
import org.sakaiproject.exception.IdUnusedException;
Expand Down Expand Up @@ -89,12 +90,12 @@ public void onSetUp() throws Exception {
// Site Service
M_ss = createMock(SiteService.class);
// Site A has SiteStats
Site siteA = new FakeSite(FakeData.SITE_A_ID, StatsManager.SITESTATS_TOOLID);
Site siteA = Mockito.spy(FakeSite.class).set(FakeData.SITE_A_ID, StatsManager.SITESTATS_TOOLID);
expect(M_ss.getSite(FakeData.SITE_A_ID)).andStubReturn(siteA);
expect(M_ss.isUserSite(FakeData.SITE_A_ID)).andStubReturn(false);
expect(M_ss.isSpecialSite(FakeData.SITE_A_ID)).andStubReturn(false);
// Site B don't have SiteStats
FakeSite siteB = new FakeSite(FakeData.SITE_B_ID);
FakeSite siteB = Mockito.spy(FakeSite.class).set(FakeData.SITE_B_ID);
expect(M_ss.getSite(FakeData.SITE_B_ID)).andStubReturn(siteB);
expect(M_ss.isUserSite(FakeData.SITE_B_ID)).andStubReturn(false);
expect(M_ss.isSpecialSite(FakeData.SITE_B_ID)).andStubReturn(false);
Expand Down Expand Up @@ -128,7 +129,7 @@ public void onSetUp() throws Exception {
@SuppressWarnings("unchecked")
@Test
public void testCollectEvent() {
FakeEvent e1 = new FakeEvent(FakeData.EVENT_CHATNEW, "/chat/msg/"+FakeData.SITE_A_ID, true, 0);
FakeEvent e1 = Mockito.spy(FakeEvent.class).set(FakeData.EVENT_CHATNEW, "/chat/msg/"+FakeData.SITE_A_ID, FakeData.SITE_A_ID, true, 0);
Assert.assertTrue(M_sum.collectEvent(e1));
Event e2 = M_sum.buildEvent(new Date(), FakeData.EVENT_CHATNEW, "/chat/msg/"+FakeData.SITE_A_ID, FakeData.SITE_A_ID, "FakeData.USER_A_ID", "session-id-a");
Assert.assertTrue(M_sum.collectEvent(e2));
Expand Down
Loading

0 comments on commit 708b4d5

Please sign in to comment.