Skip to content

Conversation

YenguiSeddik
Copy link
Contributor

No description provided.

Seddik Yengui added 4 commits September 9, 2024 16:24
Comment on lines -651 to +652
mockMvc.perform(put("/" + VERSION + "/results/" + RESULT_UUID + "/stop"
+ "?receiver=me"))
mockMvc.perform(put("/" + VERSION + "/results/" + RESULT_UUID + "/stop" + "?receiver=me")
.header(HEADER_USER_ID, "testUserId"))
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems here we manage to get the stopped notification and not the cancelfailed notification. Why ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think because we running in different thread and we wait

Copy link
Contributor

Choose a reason for hiding this comment

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

// wait for security analysis to actually run before trying to stop it
countDownLatch.await();

And because in this service we have a SecurityAnalysisProviderMock which make:

// creating a long completable future which is here to be canceled
return new CompletableFuture<SecurityAnalysisReport>().completeOnTimeout(REPORT, 3, TimeUnit.SECONDS);

then we have the time to cancel the computation and obtain a clean sa.stopped

Both of those code parts seems absent of other computation services (LF, SE,Dy, CC etc...)
Should add it

Copy link
Contributor

@AbdelHedhili AbdelHedhili left a comment

Choose a reason for hiding this comment

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

should add another test to test the cancelfailed notif no ?

Signed-off-by: Seddik Yengui <[email protected]>
Copy link
Contributor

@sBouzols sBouzols left a comment

Choose a reason for hiding this comment

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

Code Review OK

Copy link

@YenguiSeddik YenguiSeddik merged commit 330c6ed into main Sep 12, 2024
3 checks passed
@YenguiSeddik YenguiSeddik deleted the gridsuite_dependencies_32 branch September 12, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants