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

[ELY-1586] Update test suite to use certificate generation utilities #1151

Merged
merged 1 commit into from Jun 19, 2018

Conversation

justinmcook
Copy link
Contributor

@wildfly-ci
Copy link

Can one of the admins verify this patch?

@fjuma
Copy link
Contributor

fjuma commented Jun 8, 2018

This is OK to test.


@Override
public Statement apply(Statement current, Description description) {
return new Statement() {

@Override
public void evaluate() throws Throwable {
try {
setUp();
} catch (Exception e) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

If setUp() throws an exception for some reason, the empty catch block here will prevent it from being reported. Since evaluate() is already declared as throwing Throwable, the try/catch block could just be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

if (trustFile.exists() == false) {
setUp();
}
} catch (Exception e) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

The same thing would happen with this empty catch block as well. Since the outer try/catch block already catches Exception, this inner try/catch block could just be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -1,72 +0,0 @@
Certificate:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the files for 02.pem to 06.pem should also be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think src/test/resources/client.keystore should also be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the extra files, except src/test/resources/client.keystore which is needed still for the unmodified PKCS10CertificateSigningRequestTest

…ificate generation instead of using pre-generated CAs and certs
@fjuma fjuma added the +1 FJ label Jun 8, 2018
@mchoma
Copy link
Contributor

mchoma commented Jun 11, 2018

qa ack not rfe

@hkalina hkalina added the +1 HK label Jun 13, 2018
@hkalina
Copy link

hkalina commented Jun 13, 2018

This is ok to test

@hkalina
Copy link

hkalina commented Jun 13, 2018

@darranl can you say CI "ok to test"? :) (or maybe "add to whitelist" directly)

@darranl darranl added the +1 DAL label Jun 19, 2018
@darranl darranl merged commit 0c32d56 into wildfly-security:master Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants