Skip to content
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.

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

Creating an modification with wrong bus bar section now throws an exception #2516

Merged
merged 6 commits into from
Mar 29, 2023

Conversation

klesaulnier
Copy link
Contributor

Please check if the PR fulfills these requirements (please use '[x]' to check the checkboxes, or submit the PR and then click the checkboxes)

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
Creating a modification "replace tee point by voltage level on line" with wrong bus bar section is not sending an exception and provoke an unexpected behaviour

What is the new behavior (if this is a feature change)?
Creating a modification "replace tee point by voltage level on line" with wrong bus bar section is throwing an exception

…n exception

Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier@rte-france.com>
@miovd
Copy link
Contributor

miovd commented Mar 27, 2023

@archendel Could you please add a TU illustrating your point?

// even if this busbarSection is not included in the voltageLevel
// this method returns null if the busbarSectionId does not belong to the voltageLevel
private BusbarSection getBusBarSectionFromVoltageLevel(VoltageLevel voltageLevel, String busBarSectionId) {
return voltageLevel.getNodeBreakerView().getBusbarSectionStream().filter(bbs -> busBarSectionId.equals(bbs.getId())).findFirst().orElse(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if there are not that many busbar sections in a voltage level, instead of looping over the bus bar sections you'd rather do network.getBusbarSection(bbsOrBusId) and then check if busbarSection.getTerminal().getVoltageLevel() returns the expected voltage level.

Copy link
Contributor

@miovd miovd Mar 27, 2023

Choose a reason for hiding this comment

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

See comment below for even more simplification. This method can be deleted with this suggestion.

@@ -199,7 +206,7 @@ public void apply(Network network, boolean throwException,
newLine2Adder.setBus1(bus2.getId());
newLine2Adder.setConnectableBus1(bus2.getId());
} else if (topologyKind == TopologyKind.NODE_BREAKER) {
BusbarSection bbs = network.getBusbarSection(bbsOrBusId);
BusbarSection bbs = getBusBarSectionFromVoltageLevel(tappedVoltageLevel, bbsOrBusId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Acutally, here you can do tappedVoltageLevel.getNodeBreakerView().getBusbarSection(bbsOrBusId)

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 does not seem to work as expected as stated in the comment section

"tappedVoltageLevel.getNodeBreakerView().getBusbarSection(bbsOrBusId)" will return the busBarSection from the network even if it does not belong to "tappedVoltageLevel"

// even if this busbarSection is not included in the voltageLevel
// this method returns null if the busbarSectionId does not belong to the voltageLevel
private BusbarSection getBusBarSectionFromVoltageLevel(VoltageLevel voltageLevel, String busBarSectionId) {
return voltageLevel.getNodeBreakerView().getBusbarSectionStream().filter(bbs -> busBarSectionId.equals(bbs.getId())).findFirst().orElse(null);
Copy link
Contributor

@miovd miovd Mar 27, 2023

Choose a reason for hiding this comment

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

See comment below for even more simplification. This method can be deleted with this suggestion.

Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier@rte-france.com>
@klesaulnier
Copy link
Contributor Author

@archendel Could you please add a TU illustrating your point?

I've added the test illustrating this point for the NODE_BREAKER block, but I did not find any test covering the BUS_BREAKER block, is it normal ?

@miovd
Copy link
Contributor

miovd commented Mar 27, 2023

@archendel Could you please add a TU illustrating your point?

I've added the test illustrating this point for the NODE_BREAKER block, but I did not find any test covering the BUS_BREAKER block, is it normal ?

Can you check the coverage of this test replaceTeePointByVoltageLevelOnLineBbTest?

// this method returns null if the busbarSectionId does not belong to the voltageLevel
private BusbarSection getBusBarSectionFromVoltageLevel(Network network, VoltageLevel voltageLevel, String busBarSectionId) {
BusbarSection bbs = network.getBusbarSection(busBarSectionId);
if (bbs == null || !voltageLevel.equals(bbs.getTerminal().getVoltageLevel())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than equal, you should use ==/!= for voltage level

LE SAULNIER Kevin added 2 commits March 27, 2023 12:49
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier@rte-france.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier@rte-france.com>
@klesaulnier
Copy link
Contributor Author

@archendel Could you please add a TU illustrating your point?

I've added the test illustrating this point for the NODE_BREAKER block, but I did not find any test covering the BUS_BREAKER block, is it normal ?

Can you check the coverage of this test replaceTeePointByVoltageLevelOnLineBbTest?

I've found the test for the BUS_BREAKER block and added a test to cover the bug fix

@miovd
Copy link
Contributor

miovd commented Mar 28, 2023

@archendel Since the fix on getNodeBreakerView().getBusbarSection(String id) has been passed, can you use it in your PR? (you will have to merge main first)

LE SAULNIER Kevin added 2 commits March 28, 2023 16:42
…ow using it for this fix

Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier@rte-france.com>
@klesaulnier klesaulnier reopened this Mar 28, 2023
@klesaulnier
Copy link
Contributor Author

@archendel Since the fix on getNodeBreakerView().getBusbarSection(String id) has been passed, can you use it in your PR? (you will have to merge main first)

Sorry, I've closed this PR by mistake
I've made the requested changes, tests are working correctly so it seems to be fixed

Thank you

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@miovd miovd merged commit 331e4be into main Mar 29, 2023
@miovd miovd deleted the fix_replace_tee_point_by_voltage_level_on_line branch March 29, 2023 08:03
miovd pushed a commit that referenced this pull request Mar 30, 2023
…eption (#2516)

Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier@rte-france.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants