Move parameters from study-server by Tristan-WorkGH · Pull Request #81 · gridsuite/shortcircuit-server

Tristan-WorkGH · 2024-03-13T15:17:32Z

Need PR #96 to have correct diff.

sonarqubecloud · 2024-03-13T15:31:25Z

Quality Gate failed

Failed conditions
38.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

src/main/java/org/gridsuite/shortcircuit/server/ParametersController.java

src/main/java/org/gridsuite/shortcircuit/server/converter/UuidHttpConverter.java

src/main/java/org/gridsuite/shortcircuit/server/ParametersController.java

src/main/java/org/gridsuite/shortcircuit/server/dto/ShortCircuitParametersInfos.java

src/test/java/org/gridsuite/shortcircuit/server/dto/ShortCircuitParametersInfosTest.java

src/test/java/org/gridsuite/shortcircuit/server/service/AnalysisParametersTest.java

src/test/java/org/gridsuite/shortcircuit/server/service/ShortCircuitServiceTest.java

TheMaskedTurtle · 2024-06-04T15:55:36Z

src/test/java/org/gridsuite/shortcircuit/server/service/ShortCircuitWorkerServiceTest.java

+
+@ExtendWith({ MockitoExtension.class })
+@Slf4j
+class ShortCircuitWorkerServiceTest implements WithAssertions {


Can you extract that in another PR ?

done in #581, but can't really "move" out of this PR as the worker-service-test use the wrong name that I need

TheMaskedTurtle

Can you reduce a bit the unitary testing and add more integrated tests instead ? From the parameters controller endpoints to the DB for example.

src/main/java/org/gridsuite/shortcircuit/server/dto/ShortCircuitPredefinedConfiguration.java

src/main/java/org/gridsuite/shortcircuit/server/ShortCircuitController.java

src/main/java/org/gridsuite/shortcircuit/server/ShortCircuitParametersController.java

src/main/java/org/gridsuite/shortcircuit/server/entities/AnalysisParametersEntity.java

TheMaskedTurtle · 2024-06-13T15:43:54Z

src/test/java/org/gridsuite/shortcircuit/server/entities/AnalysisParametersEntityTest.java

+    void testUpdate2() {
+        AnalysisParametersEntity entity1 = new AnalysisParametersEntity();
+        AnalysisParametersEntity entity2 = new AnalysisParametersEntity(
+                ShortCircuitConstants.DEFAULT_WITH_LIMIT_VIOLATIONS,


Why don't you use these ShortCircuitConstants in the definition of default values in the entity ?

Because their the defaults of PowSyBl, not the GridSuite ones.
And I use this fact to create differences in this test.

src/test/java/org/gridsuite/shortcircuit/server/service/ShortCircuitServiceTest.java

TheMaskedTurtle · 2024-06-13T15:50:06Z

src/test/java/org/gridsuite/shortcircuit/server/service/ShortCircuitServiceTest.java

+        verify(pEntity).getPredefinedParameters();
+        verify(pEntity).isWithLimitViolations();
+        verify(pEntity).isWithVoltageResult();
+        verify(pEntity).isWithFortescueResult();
+        verify(pEntity).isWithFeederResult();
+        verify(pEntity).getStudyType();
+        verify(pEntity).getMinVoltageDropProportionalThreshold();
+        verify(pEntity).isWithLoads();
+        verify(pEntity).isWithShuntCompensators();
+        verify(pEntity).isWithVscConverterStations();
+        verify(pEntity).isWithNeutralPosition();
+        verify(pEntity, atLeastOnce()).getInitialVoltageProfileMode();


Not sure this is really necessary

It's not an important check, but necessary to pass verifyNoMoreInteractions().

TheMaskedTurtle · 2024-06-13T15:53:20Z

src/test/java/org/gridsuite/shortcircuit/server/service/ShortCircuitServiceTest.java

-            return CompletableFuture.completedFuture(this.result);
-        }
+    @Test
+    void testUpdateExistingParameters() {


In the end what do you want to test ? There are too many checks here, it feels like not all of them bring value

I added comments to explain, and I de-duplicate some code.

TheMaskedTurtle · 2024-06-13T15:56:48Z

src/test/java/org/gridsuite/shortcircuit/server/service/ShortCircuitServiceTest.java

You created a lot of test code for just few lines of code in ShortCircuitService, that's a bit of overtesting here

I just tested all cases possibles:

get+delete+update+reset * existing+non-existing in db

create + create default

I don't think there is more than these cases that can happen?

I said you overtested this

You mean too much test cases or too much checks in a test?

ayolab

we don't have the same results as before, the "withFortescueResult" should be set to true only in case of "One bus" analysis

src/main/java/org/gridsuite/shortcircuit/server/service/ShortCircuitService.java

ayolab · 2024-06-20T13:15:37Z

src/main/java/org/gridsuite/shortcircuit/server/entities/ShortCircuitParametersEntity.java

+@Getter
+@Setter
+@Entity
+@Table(name = "shortcircuit_parameters")


Suggested change

@Table(name = "shortcircuit_parameters")

@Table(name = "short_circuit_parameters")

in order for the data migration script to work, the parameters table must have the same name as the one defined in study-server

Problem fixed in gridsuite/deployment#417

sonarqubecloud · 2024-06-21T14:12:19Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mockmvc test added

Tristan-WorkGH added bug Something isn't working help wanted Extra attention is needed labels Mar 13, 2024

Tristan-WorkGH self-assigned this Mar 13, 2024

Tristan-WorkGH force-pushed the move_parameters branch from e12fd7d to 8147f0c Compare May 23, 2024 00:37

Tristan-WorkGH mentioned this pull request May 23, 2024

Move shortcircuit parameters gridsuite/study-server#569

Merged

Tristan-WorkGH requested a review from ayolab May 23, 2024 01:12

Tristan-WorkGH force-pushed the move_parameters branch from 828480e to 3ac7d11 Compare June 4, 2024 07:56

TheMaskedTurtle requested changes Jun 4, 2024

View reviewed changes

BIG REDO SQUASH!

c470c99

Tristan-WorkGH force-pushed the move_parameters branch from 4805ebf to c470c99 Compare June 13, 2024 06:44

review

7bcee84

TheMaskedTurtle previously requested changes Jun 13, 2024

View reviewed changes

Tristan-WorkGH added 4 commits June 14, 2024 13:53

review

86dfba8

Merge remote-tracking branch 'origin/main' into move_parameters

2120011

review

bf4715e

Merge remote-tracking branch 'origin/main' into move_parameters

3ad7b5c

ayolab requested changes Jun 19, 2024

View reviewed changes

src/main/java/org/gridsuite/shortcircuit/server/service/ShortCircuitService.java Outdated Show resolved Hide resolved

src/main/java/org/gridsuite/shortcircuit/server/service/ShortCircuitService.java Outdated Show resolved Hide resolved

Tristan-WorkGH added 4 commits June 19, 2024 15:25

Fix tests

7dd5946

Remove unused FortescueResult from parameters

4997bd5

simplify

4fdc3ef

review

9ffd91e

Tristan-WorkGH requested a review from ayolab June 19, 2024 14:10

ayolab approved these changes Jun 19, 2024

View reviewed changes

Tristan-WorkGH requested a review from TheMaskedTurtle June 19, 2024 15:00

ayolab requested changes Jun 20, 2024

View reviewed changes

Tristan-WorkGH added 4 commits June 20, 2024 17:37

clarify new()

132fb50

review

48a5b21

revert

8af2c18

clean comments

25491b7

jonenst approved these changes Jun 21, 2024

View reviewed changes

Tristan-WorkGH requested review from TheMaskedTurtle and ayolab and removed request for TheMaskedTurtle June 21, 2024 14:20

ayolab approved these changes Jun 21, 2024

View reviewed changes

Tristan-WorkGH merged commit 5c3590b into main Jun 21, 2024

Tristan-WorkGH deleted the move_parameters branch June 21, 2024 14:29

Tristan-WorkGH mentioned this pull request Jun 24, 2024

Fix non null columns in liquibase of #81 #98

Merged

	@Table(name = "shortcircuit_parameters")
	@Table(name = "short_circuit_parameters")

Conversation

Tristan-WorkGH commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Mar 13, 2024

Quality Gate failed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tristan-WorkGH Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheMaskedTurtle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tristan-WorkGH Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tristan-WorkGH Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayolab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 21, 2024

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Tristan-WorkGH commented Mar 13, 2024 •

edited

Loading

Tristan-WorkGH Jun 10, 2024 •

edited

Loading

Tristan-WorkGH Jun 14, 2024 •

edited

Loading

Tristan-WorkGH Jun 14, 2024 •

edited

Loading