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

Apply idea code analysis suggestions Service*-End #2194

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mocenas
Copy link
Contributor

@mocenas mocenas commented Nov 14, 2024

Summary

Apply idea code changes to modules Service* to the end. Mostly replacing deprecated API and code simplification.

Please select the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • Dependency update
  • Refactoring
  • Backport
  • New scenario (non-breaking change which adds functionality)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@mocenas
Copy link
Contributor Author

mocenas commented Nov 14, 2024

run tests

@github-actions github-actions bot added the triage/flaky-test Signal that flaky tests were detected during CI run label Nov 14, 2024
Copy link

Following jobs contain at least one flaky test:

  • PR - Linux - JVM build - Latest Version

Run summary: https://github.com/quarkus-qe/quarkus-test-suite/actions/runs/11841077129?pr=2194

Flaky tests:


io.quarkus.ts.sqldb.panacheflyway.dbpool.AgroalPoolTest.idleTimeoutTest

  • Failure message: agroalCheckIdleTimeout: Expected 1 active connections ==> expected: <1> but was: <3>
  • Failed in jobs:
    • PR - Linux - JVM build - Latest Version
Failure stacktrace
org.opentest4j.AssertionFailedError: agroalCheckIdleTimeout: Expected 1 active connections ==> expected: <1> but was: <3>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertEquals.failNotEqual(AssertEquals.java:197)
	at org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
	at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:672)
	at io.quarkus.ts.sqldb.panacheflyway.dbpool.AgroalPoolTest.idleTimeoutTest(AgroalPoolTest.java:102)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.junit.QuarkusTestExtension.runExtensionMethod(QuarkusTestExtension.java:966)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestMethod(QuarkusTestExtension.java:816)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)


@mocenas
Copy link
Contributor Author

mocenas commented Nov 15, 2024

There are a lot of failures in OCP tests. Mostly failures with image building/pulling. Seems like some problem with OCP. I will run the tests again.

@mocenas
Copy link
Contributor Author

mocenas commented Nov 15, 2024

run tests

@gtroitsk
Copy link
Member

@mocenas can you assign this review to someone else please ?

@mocenas mocenas requested review from michalvavrik and removed request for gtroitsk November 15, 2024 10:11
@mocenas
Copy link
Contributor Author

mocenas commented Nov 15, 2024

@mocenas can you assign this review to someone else please ?

OK, changed the reviewer this to @michalvavrik

@mjurc
Copy link
Member

mjurc commented Nov 15, 2024

run tests

return false;
return true;
return other.title == null;
} else
Copy link
Member

@michalvavrik michalvavrik Nov 15, 2024

Choose a reason for hiding this comment

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

FYI in QE FW there is a checkstyle rule that you need to use curly brackets. This was part of the original code as well and validation succeeded so np.

@@ -23,7 +22,7 @@ public abstract class AbstractSpringWebRestReactiveIT {

@Test
public void whenGetNotExistBookById_thenNotFound() {
final Response response = getApp().given().get(API_ROOT + "/" + randomNumeric(4));
final Response response = getApp().given().get(API_ROOT + "/" + insecure().nextNumeric(4));
Copy link
Member

Choose a reason for hiding this comment

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

Java 17 has RandomGenerator, that would be my preference. Anyway, my question: I am not sure why is using one org.apache.commons.lang3.RandomStringUtils method over another method better. Is that because the original method was deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

methods for RandomStringUtils randomNumber and randomAlphabetic are deprecated. Construct I use is one of the recommended replacements. I used insecure because we don't need cryptographically secured randomness here.

Copy link
Member

Choose a reason for hiding this comment

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

ok, thanks

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

I think changes are OK, not really valuable but we agreed to do that so thanks for the work done. I don't really have requests for changes as long as OCP run will be green or inspected properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/flaky-test Signal that flaky tests were detected during CI run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants