-
Notifications
You must be signed in to change notification settings - Fork 612
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
ISPN-8700 Ensure that REST test threads include the test name #5722
ISPN-8700 Ensure that REST test threads include the test name #5722
Conversation
@@ -76,7 +76,7 @@ public void shouldRejectNotValidAuthorizationString() throws Exception { | |||
|
|||
BasicAuthenticator basicAuthenticator = new BasicAuthenticator(securityDomainMock, "ApplicationRealm"); | |||
|
|||
restServer = RestServerHelper.defaultRestServer().withAuthenticator(basicAuthenticator).start(); | |||
restServer = RestServerHelper.defaultRestServer().withAuthenticator(basicAuthenticator).start(this.getTestName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about getting current's thread name (which would be probably a Surefire fork) rather than grabbing test name explicitly. This way the start method wouldn't need to accept parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this is a test helper, so I'm not bothered about parameters. I've aligned the naming to use the AbstractInfinispanTest strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to keep the parameter.
However, getTestName()
returns the full class name, so I suggest using TestResourceTracker.getCurrentTestShortName()
instead. BTW, the tests would still have to extend AbstractInfinispanTest
for the test thread's name to be set.
f8b81cd
to
5e06437
Compare
Fixed |
pulling |
Actually, this PR was run 3x by CI, and the failures were always high (> 40), could you check @tristantarrant ? |
Weird that a PR that only touches server/rest/ files has so many failures on core... |
@tristantarrant Could you rebase? Let's wait for another CI run |
5e06437
to
98d59d5
Compare
rebased |
CI seems stuck, with lots of failures |
restarted the build |
Something weird with this branch. Creating a new PR. |
Fixes the fact that RestAccessLoggingTest occasionally fails when running concurrently with other tests