Skip to content

Commit 1ca47fc

Browse files
henchaveshenchaves
authored
Merge pull request #1333 from Giskard-AI/feature/gsk-1552-test-suite-isnt-listed-if-it-doesnt-contain-a-name
[GSK-1552] Fix test suite not being listed on Testing tab
2 parents 1873da8 + 40ff904 commit 1ca47fc

4 files changed

Lines changed: 147 additions & 61 deletions

File tree

‎backend/src/main/java/ai/giskard/service/TestSuiteService.java‎

Lines changed: 74 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@
3535

3636
import static ai.giskard.web.rest.errors.Entity.TEST_SUITE;
3737

38-
3938
@Service
4039
@RequiredArgsConstructor
4140
public class TestSuiteService {
@@ -49,36 +48,52 @@ public class TestSuiteService {
4948
private final MLWorkerWSCommService mlWorkerWSCommService;
5049
private final TestFunctionRepository testFunctionRepository;
5150

51+
public Long saveTestSuite(String projectKey, TestSuiteDTO dto) {
52+
if (dto.getName() == null || dto.getName().isBlank()) {
53+
throw new IllegalArgumentException("Test suite name cannot be blank");
54+
}
55+
56+
if (dto.getProjectKey() == null) {
57+
dto.setProjectKey(projectKey);
58+
}
59+
60+
TestSuite savedSuite = testSuiteRepository.save(giskardMapper.fromDTO(dto));
61+
return savedSuite.getId();
62+
}
63+
5264
@Transactional(readOnly = true)
5365
public Map<String, RequiredInputDTO> getSuiteInputs(Long projectId, Long suiteId) {
5466
TestSuite suite = testSuiteRepository.findOneByProjectIdAndId(projectId, suiteId);
5567

5668
Map<String, RequiredInputDTO> res = new HashMap<>();
5769

5870
suite.getTests().forEach(test -> {
59-
ImmutableMap<String, FunctionInput> providedInputs = Maps.uniqueIndex(test.getFunctionInputs(), FunctionInput::getName);
71+
ImmutableMap<String, FunctionInput> providedInputs = Maps.uniqueIndex(test.getFunctionInputs(),
72+
FunctionInput::getName);
6073

6174
test.getTestFunction().getArgs().stream()
62-
.filter(a -> !a.isOptional())
63-
.forEach(a -> {
64-
String name = null;
65-
boolean isShared = false;
66-
if (!providedInputs.containsKey(a.getName())) {
67-
name = a.getName();
68-
} else if (providedInputs.get(a.getName()).isAlias()) {
69-
name = providedInputs.get(a.getName()).getValue();
70-
isShared = true;
71-
}
72-
if (name != null) {
73-
if (res.containsKey(name) && !a.getType().equals(res.get(name).getType())) {
74-
throw new IllegalArgumentException("Variable with name %s is declared as %s and %s at the same time".formatted(a.getName(), res.get(a.getName()), a.getType()));
75-
} else if (res.containsKey(name)) {
76-
res.get(name).setSharedInput(isShared || res.get(name).isSharedInput());
77-
} else {
78-
res.put(name, new RequiredInputDTO(a.getType(), isShared));
75+
.filter(a -> !a.isOptional())
76+
.forEach(a -> {
77+
String name = null;
78+
boolean isShared = false;
79+
if (!providedInputs.containsKey(a.getName())) {
80+
name = a.getName();
81+
} else if (providedInputs.get(a.getName()).isAlias()) {
82+
name = providedInputs.get(a.getName()).getValue();
83+
isShared = true;
84+
}
85+
if (name != null) {
86+
if (res.containsKey(name) && !a.getType().equals(res.get(name).getType())) {
87+
throw new IllegalArgumentException(
88+
"Variable with name %s is declared as %s and %s at the same time"
89+
.formatted(a.getName(), res.get(a.getName()), a.getType()));
90+
} else if (res.containsKey(name)) {
91+
res.get(name).setSharedInput(isShared || res.get(name).isSharedInput());
92+
} else {
93+
res.put(name, new RequiredInputDTO(a.getType(), isShared));
94+
}
7995
}
80-
}
81-
});
96+
});
8297
});
8398

8499
return res;
@@ -93,7 +108,7 @@ public UUID scheduleTestSuiteExecution(Long projectId, Long suiteId, List<Functi
93108
execution.setInputs(inputs.stream().map(giskardMapper::fromDTO).toList());
94109

95110
Map<String, String> suiteInputs = getSuiteInputs(projectId, suiteId).entrySet().stream()
96-
.collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().getType()));
111+
.collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().getType()));
97112

98113
verifyAllInputProvided(inputs, testSuite, suiteInputs);
99114

@@ -105,8 +120,8 @@ public UUID scheduleTestSuiteExecution(Long projectId, Long suiteId, List<Functi
105120
}
106121

107122
public TestSuiteExecution tryTestSuiteExecution(TestSuite testSuite,
108-
Map<String, String> suiteInputs,
109-
List<FunctionInputDTO> inputs) {
123+
Map<String, String> suiteInputs,
124+
List<FunctionInputDTO> inputs) {
110125
TestSuiteExecution execution = new TestSuiteExecution(testSuite);
111126
execution.setInputs(inputs.stream().map(giskardMapper::fromDTO).toList());
112127

@@ -118,52 +133,53 @@ public TestSuiteExecution tryTestSuiteExecution(TestSuite testSuite,
118133
}
119134

120135
public static void verifyAllInputProvided(List<FunctionInputDTO> providedInputs,
121-
TestSuite testSuite,
122-
Map<String, String> requiredInputs) {
136+
TestSuite testSuite,
137+
Map<String, String> requiredInputs) {
123138
Set<String> names = providedInputs.stream().map(FunctionInputDTO::getName).collect(Collectors.toSet());
124139

125140
List<String> missingInputs = requiredInputs.keySet().stream()
126-
.filter(requiredInput -> !names.contains(requiredInput))
127-
.toList();
141+
.filter(requiredInput -> !names.contains(requiredInput))
142+
.toList();
128143
if (!missingInputs.isEmpty()) {
129144
throw new IllegalArgumentException("Inputs '%s' required to execute test suite %s"
130-
.formatted(String.join(", ", missingInputs), testSuite.getName()));
145+
.formatted(String.join(", ", missingInputs), testSuite.getName()));
131146
}
132147
}
133148

134149
public TestSuiteDTO updateTestInputs(long suiteId, long testId, List<FunctionInputDTO> inputs) {
135150
TestSuite testSuite = testSuiteRepository.getMandatoryById(suiteId);
136151

137152
SuiteTest test = testSuite.getTests().stream()
138-
.filter(t -> testId == t.getId())
139-
.findFirst().orElseThrow(() -> new EntityNotFoundException(TEST_SUITE, testId));
153+
.filter(t -> testId == t.getId())
154+
.findFirst().orElseThrow(() -> new EntityNotFoundException(TEST_SUITE, testId));
140155

141156
verifyAllInputExists(inputs, test);
142157

143158
test.getFunctionInputs().clear();
144159
test.getFunctionInputs().addAll(inputs.stream()
145-
.filter(i -> i.getValue() != null)
146-
.map(giskardMapper::fromDTO)
147-
.toList());
160+
.filter(i -> i.getValue() != null)
161+
.map(giskardMapper::fromDTO)
162+
.toList());
148163

149164
return giskardMapper.toDTO(testSuiteRepository.save(testSuite));
150165
}
151166

152167
private void verifyAllInputExists(List<FunctionInputDTO> providedInputs,
153-
SuiteTest test) {
168+
SuiteTest test) {
154169
Set<String> requiredInputs = test.getTestFunction().getArgs().stream()
155-
.map(FunctionArgument::getName)
156-
.collect(Collectors.toSet());
170+
.map(FunctionArgument::getName)
171+
.collect(Collectors.toSet());
157172

158173
List<String> nonExistingInputs = providedInputs.stream()
159-
.map(FunctionInputDTO::getName)
160-
.filter(providedInput -> !requiredInputs.contains(providedInput))
161-
.toList();
174+
.map(FunctionInputDTO::getName)
175+
.filter(providedInput -> !requiredInputs.contains(providedInput))
176+
.toList();
162177

163178
if (!nonExistingInputs.isEmpty()) {
164179
throw new IllegalArgumentException("Inputs '%s' does not exists for test %s"
165-
.formatted(String.join(", ", nonExistingInputs),
166-
Objects.requireNonNullElse(test.getTestFunction().getDisplayName(), test.getTestFunction().getName())));
180+
.formatted(String.join(", ", nonExistingInputs),
181+
Objects.requireNonNullElse(test.getTestFunction().getDisplayName(),
182+
test.getTestFunction().getName())));
167183
}
168184
}
169185

@@ -173,7 +189,7 @@ public Path resolvedMetadataPath(Path temporaryMetadataDir, String entityName) {
173189

174190
public TestSuite addTestToSuite(long suiteId, SuiteTestDTO suiteTestDTO) {
175191
TestSuite suite = testSuiteRepository.findById(suiteId)
176-
.orElseThrow(() -> new EntityNotFoundException(TEST_SUITE, suiteId));
192+
.orElseThrow(() -> new EntityNotFoundException(TEST_SUITE, suiteId));
177193

178194
SuiteTest suiteTest = giskardMapper.fromDTO(suiteTestDTO);
179195
suiteTest.setSuite(suite);
@@ -188,30 +204,30 @@ public Long generateTestSuite(String projectKey, GenerateTestSuiteDTO dto) {
188204
MLWorkerID workerID = project.isUsingInternalWorker() ? MLWorkerID.INTERNAL : MLWorkerID.EXTERNAL;
189205
if (mlWorkerWSService.isWorkerConnected(workerID)) {
190206
MLWorkerWSGenerateTestSuiteParamDTO param = MLWorkerWSGenerateTestSuiteParamDTO.builder()
191-
.projectKey(projectKey)
192-
.build();
207+
.projectKey(projectKey)
208+
.build();
193209

194210
List<MLWorkerWSSuiteInputDTO> inputs = dto.getInputs()
195-
.stream()
196-
.map(TestSuiteService::generateSuiteInputWS)
197-
.toList();
211+
.stream()
212+
.map(TestSuiteService::generateSuiteInputWS)
213+
.toList();
198214
param.setInputs(inputs);
199215

200216
MLWorkerWSBaseDTO result = mlWorkerWSCommService.performAction(
201-
workerID,
202-
MLWorkerWSAction.GENERATE_TEST_SUITE,
203-
param
204-
);
217+
workerID,
218+
MLWorkerWSAction.GENERATE_TEST_SUITE,
219+
param);
205220
if (result instanceof MLWorkerWSGenerateTestSuiteDTO response) {
206221
TestSuite suite = new TestSuite();
207222
suite.setProject(project);
208223
suite.setName(dto.getName());
209224
suite.setFunctionInputs(dto.getSharedInputs().stream()
210-
.map(giskardMapper::fromDTO)
211-
.toList());
225+
.map(giskardMapper::fromDTO)
226+
.toList());
212227
suite.getTests().addAll(response.getTests().stream()
213-
.map(test -> new SuiteTest(suite, test, testFunctionRepository.getMandatoryById(UUID.fromString(test.getTestUuid()))))
214-
.toList());
228+
.map(test -> new SuiteTest(suite, test,
229+
testFunctionRepository.getMandatoryById(UUID.fromString(test.getTestUuid()))))
230+
.toList());
215231

216232
return testSuiteRepository.save(suite).getId();
217233
} else if (result instanceof MLWorkerWSErrorDTO error) {
@@ -254,13 +270,12 @@ public TestSuiteDTO updateTestSuite(long suiteId, TestSuiteDTO testSuiteDTO) {
254270
testSuite.setName(testSuiteDTO.getName());
255271
testSuite.getFunctionInputs().clear();
256272
testSuite.getFunctionInputs().addAll(testSuiteDTO.getFunctionInputs().stream()
257-
.map(giskardMapper::fromDTO)
258-
.toList());
273+
.map(giskardMapper::fromDTO)
274+
.toList());
259275

260276
return giskardMapper.toDTO(testSuiteRepository.save(testSuite));
261277
}
262278

263-
264279
public void deleteTestSuite(long suiteId) {
265280
testSuiteRepository.deleteById(suiteId);
266281
}

‎backend/src/main/java/ai/giskard/web/rest/controllers/testing/TestSuiteController.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,7 @@ public class TestSuiteController {
3838
@PostMapping("project/{projectKey}/suites")
3939
@PreAuthorize("@permissionEvaluator.canWriteProjectKey(#projectKey)")
4040
public Long saveTestSuite(@PathVariable("projectKey") @NotNull String projectKey, @Valid @RequestBody TestSuiteDTO dto) {
41-
TestSuite savedSuite = testSuiteRepository.save(giskardMapper.fromDTO(dto));
42-
return savedSuite.getId();
41+
return testSuiteService.saveTestSuite(projectKey, dto);
4342
}
4443

4544
@PostMapping("project/{projectKey}/suites/generate")

‎backend/src/test/java/ai/giskard/web/rest/testing/TestSuiteControllerIT.java‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
package ai.giskard.web.rest.testing;
2+
3+
import ai.giskard.IntegrationTest;
4+
import ai.giskard.domain.Project;
5+
import ai.giskard.repository.ProjectRepository;
6+
import ai.giskard.security.AuthoritiesConstants;
7+
import ai.giskard.service.InitService;
8+
import ai.giskard.web.dto.TestSuiteDTO;
9+
import com.fasterxml.jackson.databind.ObjectMapper;
10+
import org.junit.jupiter.api.Test;
11+
import org.springframework.beans.factory.annotation.Autowired;
12+
import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc;
13+
import org.springframework.security.test.context.support.WithMockUser;
14+
import org.springframework.test.web.servlet.MockMvc;
15+
import org.springframework.transaction.annotation.Transactional;
16+
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
17+
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
18+
import org.springframework.http.MediaType;
19+
20+
import java.util.ArrayList;
21+
22+
23+
@AutoConfigureMockMvc
24+
@IntegrationTest
25+
@WithMockUser(username = "admin", authorities = AuthoritiesConstants.ADMIN)
26+
class TestSuiteControllerIT {
27+
28+
@Autowired
29+
private MockMvc restUserMockMvc;
30+
31+
@Autowired
32+
ProjectRepository projectRepository;
33+
34+
35+
@Autowired
36+
private InitService initService;
37+
38+
@Test
39+
@Transactional
40+
void saveTestSuite() throws Exception {
41+
Project project = projectRepository.getOneByName(initService.getProjectByCreatorLogin("admin"));
42+
43+
TestSuiteDTO testSuiteDTO = new TestSuiteDTO();
44+
testSuiteDTO.setName("My test suite");
45+
testSuiteDTO.setProjectKey(project.getKey());
46+
testSuiteDTO.setTests(new ArrayList<>());
47+
48+
restUserMockMvc.perform(post("/api/v2/testing/project/" + project.getKey() + "/suites")
49+
.contentType(MediaType.APPLICATION_JSON)
50+
.content(new ObjectMapper().writeValueAsString(testSuiteDTO)))
51+
.andExpect(status().isOk());
52+
53+
}
54+
55+
@Test
56+
@Transactional
57+
void saveTestSuiteWithEmptyName() throws Exception {
58+
Project project = projectRepository.getOneByName(initService.getProjectByCreatorLogin("admin"));
59+
60+
TestSuiteDTO testSuiteDTO = new TestSuiteDTO();
61+
testSuiteDTO.setProjectKey(project.getKey());
62+
testSuiteDTO.setTests(new ArrayList<>());
63+
64+
restUserMockMvc.perform(post("/api/v2/testing/project/" + project.getKey() + "/suites")
65+
.contentType(MediaType.APPLICATION_JSON)
66+
.content(new ObjectMapper().writeValueAsString(testSuiteDTO)))
67+
.andExpect(status().isBadRequest());
68+
69+
}
70+
}

‎python-client/giskard/core/suite.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -361,6 +361,8 @@ def upload(self, client: GiskardClient, project_key: str):
361361
:param project_key: The key of the project that the test suite belongs to.
362362
:return: The current instance of the test Suite to allow chained call.
363363
"""
364+
if self.name is None:
365+
self.name = "Unnamed test suite"
364366
self.id = client.save_test_suite(self.to_dto(client, project_key))
365367
project_id = client.get_project(project_key).project_id
366368
print(f"Test suite has been saved: {client.host_url}/main/projects/{project_id}/test-suite/{self.id}/overview")

0 commit comments

Comments
 (0)