Skip to content

Conversation

@Seppli11
Copy link
Contributor

@Seppli11 Seppli11 commented Dec 13, 2024

@Seppli11 Seppli11 force-pushed the SONARPY-2462 branch 4 times, most recently from 46c733b to b230586 Compare December 13, 2024 17:26
Copy link
Contributor

Choose a reason for hiding this comment

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

Was that test not run before? And it's actually failing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, it wasn't run before. I've tried running this test on master and it failed as well. So, it seems that it failing is not connected to changes made in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Running the test locally, the issues do look legit. We could update the assertions (I believe asserting an empty list of issue was a mistake).
We can do that here or in a separate PR, as you prefer.

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'll change the assertion. I think it should be doable in this PR

@Seppli11 Seppli11 force-pushed the SONARPY-2462 branch 2 times, most recently from e9613cc to cbaa5a5 Compare December 16, 2024 14:11
Copy link
Contributor

@guillaume-dequenne guillaume-dequenne left a comment

Choose a reason for hiding this comment

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

Small comment on unsued method.
LGTM otherwise, great work!

return new ConcurrentOrchestratorExtensionBuilder(Configuration.createEnv());
}

public static ConcurrentOrchestratorExtensionBuilder builder(Configuration config) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unused, do we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. You're right, we don't need it. thx

@sonarqube-next
Copy link

@Seppli11 Seppli11 merged commit e917bf9 into master Dec 16, 2024
10 checks passed
@Seppli11 Seppli11 deleted the SONARPY-2462 branch December 16, 2024 15:01
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.

2 participants