Skip to content

Move parameters from study-server #81

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

Jump to bottom
Merged
merged 14 commits into from
Jun 21, 2024
Merged

Move parameters from study-server #81

merged 14 commits into from
Jun 21, 2024

Conversation

Tristan-WorkGH
Copy link
Contributor

@Tristan-WorkGH Tristan-WorkGH commented Mar 13, 2024 •
edited
Loading

Need PR #96 to have correct diff.

@Tristan-WorkGH Tristan-WorkGH added bug Something isn't working help wanted Extra attention is needed labels Mar 13, 2024
@Tristan-WorkGH Tristan-WorkGH self-assigned this Mar 13, 2024
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

@Tristan-WorkGH Tristan-WorkGH mentioned this pull request May 23, 2024
Merged
@Tristan-WorkGH Tristan-WorkGH requested a review from ayolab May 23, 2024 01:12
TheMaskedTurtle
TheMaskedTurtle requested changes Jun 4, 2024
View reviewed changes
src/test/java/org/gridsuite/shortcircuit/server/service/ShortCircuitWorkerServiceTest.java Outdated

@ExtendWith({ MockitoExtension.class })
@Slf4j
class ShortCircuitWorkerServiceTest implements WithAssertions {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you extract that in another PR ?

Copy link
Contributor Author

@Tristan-WorkGH Tristan-WorkGH Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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
TheMaskedTurtle previously requested changes Jun 13, 2024
View reviewed changes
Copy link
Contributor

@TheMaskedTurtle TheMaskedTurtle left a comment

Choose a reason for hiding this comment

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

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/test/java/org/gridsuite/shortcircuit/server/entities/AnalysisParametersEntityTest.java Outdated
void testUpdate2() {
AnalysisParametersEntity entity1 = new AnalysisParametersEntity();
AnalysisParametersEntity entity2 = new AnalysisParametersEntity(
ShortCircuitConstants.DEFAULT_WITH_LIMIT_VIOLATIONS,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Outdated
Comment on lines 111 to 122
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();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this is really necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

src/test/java/org/gridsuite/shortcircuit/server/service/ShortCircuitServiceTest.java
return CompletableFuture.completedFuture(this.result);
}
@Test
void testUpdateExistingParameters() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Tristan-WorkGH Tristan-WorkGH Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

src/test/java/org/gridsuite/shortcircuit/server/service/ShortCircuitServiceTest.java
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Tristan-WorkGH Tristan-WorkGH Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

I said you overtested this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

ayolab
ayolab requested changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@ayolab ayolab left a comment

Choose a reason for hiding this comment

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

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

@Tristan-WorkGH Tristan-WorkGH requested a review from ayolab June 19, 2024 14:10
ayolab
ayolab requested changes Jun 20, 2024
View reviewed changes
src/main/java/org/gridsuite/shortcircuit/server/entities/ShortCircuitParametersEntity.java
@Getter
@Setter
@Entity
@Table(name = "shortcircuit_parameters")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem fixed in gridsuite/deployment#417

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed 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

@jonenst jonenst dismissed TheMaskedTurtle’s stale review June 21, 2024 14:13

mockmvc test added

@Tristan-WorkGH Tristan-WorkGH requested review from ayolab and TheMaskedTurtle and removed request for TheMaskedTurtle June 21, 2024 14:20
@Tristan-WorkGH Tristan-WorkGH merged commit 5c3590b into main Jun 21, 2024
3 checks passed
@Tristan-WorkGH Tristan-WorkGH deleted the move_parameters branch June 21, 2024 14:29
@Tristan-WorkGH Tristan-WorkGH mentioned this pull request Jun 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonenst jonenst jonenst approved these changes

@ayolab ayolab ayolab approved these changes

@TheMaskedTurtle TheMaskedTurtle Awaiting requested review from TheMaskedTurtle

Assignees

@Tristan-WorkGH Tristan-WorkGH

Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Tristan-WorkGH @jonenst @TheMaskedTurtle @ayolab