Skip to content

Fix unit tests not running in feast.core package. #883

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.

Sign up for GitHub

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

Merged
feast-ci-bot merged 6 commits into from
Jul 17, 2020
Merged

Fix unit tests not running in feast.core package. #883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7e8ed29
Remove Junit 5 dependency from Feast Core
mrzzy Jul 15, 2020
7befe44
Change CoreServiceAuthTest imports to Junit 4 imports
mrzzy Jul 15, 2020
f2ecb9f
Fix feast core's AccessManagementServiceTest unit test
mrzzy Jul 15, 2020
bae446f
Fix CoreServiceAuthTest not following test conventions
mrzzy Jul 15, 2020
1319fe9
Remove debug print statement
mrzzy Jul 15, 2020
ddd8bd5
Insert default project into DB via migration instead of in AccessMana…
mrzzy Jul 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions core/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,11 +315,5 @@
<version>5.1.3.RELEASE</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter</artifactId>
<version>RELEASE</version>
<scope>test</scope>
</dependency>
</dependencies>
</project>
5 changes: 1 addition & 4 deletions core/src/main/java/feast/core/service/AccessManagementService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ public class AccessManagementService {
private AuthorizationProvider authorizationProvider;
private ProjectRepository projectRepository;

// TODO: Remove duplication of constructor
Copy link
Copy Markdown
Collaborator Author

@mrzzy mrzzy Jul 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dr3s is it possible to remove the ObjectProvider here to remove the code duplication in two nearly identical AccessManagementService() constructors? This makes the code really error prone.
(:disappointed_relieved: I was stuck debugging because I didn't realize there were two constructors.)
It seems that one constructor is used by Spring and one used by tests.

public AccessManagementService(
FeastProperties feastProperties,
ProjectRepository projectRepository,
Expand All @@ -57,10 +58,6 @@ public AccessManagementService(
ProjectRepository projectRepository,
ObjectProvider<AuthorizationProvider> authorizationProvider) {
this.projectRepository = projectRepository;
// create default project if it does not yet exist.
if (!projectRepository.existsById(Project.DEFAULT_NAME)) {
this.createProject(Project.DEFAULT_NAME);
}
this.authorizationProvider = authorizationProvider.getIfUnique();
this.securityProperties = feastProperties.getSecurity();
}
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/resources/db/migration/V2.6__Create_Project_If_Not_Exist.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
-- SQL migration to migrate create 'default' project if it does not already exist.
INSERT INTO projects
(name, archived)
SELECT 'default', false
WHERE NOT EXISTS (
SELECT name FROM projects WHERE name='default'
);
12 changes: 6 additions & 6 deletions core/src/test/java/feast/core/grpc/CoreServiceAuthTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
*/
package feast.core.grpc;

import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.Assert.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.doReturn;
Expand Down Expand Up @@ -50,15 +50,15 @@
import java.time.Instant;
import java.util.Arrays;
import java.util.HashMap;
import org.junit.jupiter.api.Test;
import org.junit.Test;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.context.SecurityContext;
import org.springframework.security.core.context.SecurityContextHolder;

class CoreServiceAuthTest {
public class CoreServiceAuthTest {

private CoreServiceImpl coreService;
private AccessManagementService accessManagementService;
Expand All @@ -69,7 +69,7 @@ class CoreServiceAuthTest {
@Mock private StatsService statsService;
@Mock private JobService jobService;

CoreServiceAuthTest() {
public CoreServiceAuthTest() {
MockitoAnnotations.initMocks(this);
SecurityProperties.AuthorizationProperties authProp =
new SecurityProperties.AuthorizationProperties();
Expand All @@ -86,7 +86,7 @@ class CoreServiceAuthTest {
}

@Test
void cantApplyFeatureSetIfNotProjectMember() throws InvalidProtocolBufferException {
public void shouldNotApplyFeatureSetIfNotProjectMember() throws InvalidProtocolBufferException {

String project = "project1";
Authentication auth = mock(Authentication.class);
Expand All @@ -112,7 +112,7 @@ void cantApplyFeatureSetIfNotProjectMember() throws InvalidProtocolBufferExcepti
}

@Test
void canApplyFeatureSetIfProjectMember() throws InvalidProtocolBufferException {
public void shouldApplyFeatureSetIfProjectMember() throws InvalidProtocolBufferException {

String project = "project1";
Authentication auth = mock(Authentication.class);
Expand Down
17 changes: 6 additions & 11 deletions core/src/test/java/feast/core/service/AccessManagementServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
*/
package feast.core.service;

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
Expand All @@ -41,7 +40,6 @@
public class AccessManagementServiceTest {

@Mock private ProjectRepository projectRepository;

@Rule public final ExpectedException expectedException = ExpectedException.none();

private AccessManagementService accessManagementService;
Expand All @@ -57,23 +55,19 @@ public void setUp() {
sp.setAuthorization(authProp);
FeastProperties feastProperties = new FeastProperties();
feastProperties.setSecurity(sp);

accessManagementService =
new AccessManagementService(
feastProperties, projectRepository, mock(AuthorizationProvider.class));
}

@Test
public void testDefaultProjectCreateInConstructor() {
verify(this.projectRepository).saveAndFlush(new Project(Project.DEFAULT_NAME));
}

@Test
public void shouldCreateProjectIfItDoesntExist() {
String projectName = "project1";
Project project = new Project(projectName);
when(projectRepository.saveAndFlush(any(Project.class))).thenReturn(project);
when(projectRepository.saveAndFlush(project)).thenReturn(project);
accessManagementService.createProject(projectName);
verify(projectRepository, times(1)).saveAndFlush(any());
verify(projectRepository, times(1)).saveAndFlush(project);
}

@Test(expected = IllegalArgumentException.class)
Expand All @@ -86,9 +80,10 @@ public void shouldNotCreateProjectIfItExist() {
@Test
public void shouldArchiveProjectIfItExists() {
String projectName = "project1";
when(projectRepository.findById(projectName)).thenReturn(Optional.of(new Project(projectName)));
Project project = new Project(projectName);
when(projectRepository.findById(projectName)).thenReturn(Optional.of(project));
accessManagementService.archiveProject(projectName);
verify(projectRepository, times(1)).saveAndFlush(any(Project.class));
verify(projectRepository, times(1)).saveAndFlush(project);
}

@Test
Expand Down