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

Remove redundant integration test runs #364

Conversation

tjcelaya
Copy link
Contributor

@tjcelaya tjcelaya commented Oct 23, 2017

Overview

Resolves #336 and adds a it.dryrun property that can be used to list the tests that would run with their respective TestNG params. Unfortunately it looks like TestNG doesn't completely support filtering a test suite by groups so we need to be more explicit about suite definition.

Output of it.dryrun using current testng-it.xml (with some fixes to allow it to be used with TestListingInterceptor:

DRY-RUN: Listing [603] tests that would have run

Count of test cases that would run using new testng-it.xml:

DRY-RUN: Listing [405] tests that would have run

Lists for both are here. Note that the dry-run-after list is slightly inaccurate since it shows different values for encryptionCipher. Other changes in this branch include using encryptionCipher so a dry-run listing of master would show AES128/CBC/PKCS5Padding for all non-null ciphers in that list.

Changes

  • change usingEncryption (boolean) to just encryptionCipher (string). omission (null) disables encryption
  • add a new constructor for IntegrationTestConfigContext which handles the above
  • switch from @BeforeClass with @Parameters to test constructors with just @Parameters. Whenever SkipException was thrown or putDirectory was called in a test which needed @Parameters a minimal method was created for @BeforeClass. The goal here was to make it safe to instantiate test classes without incurring side-effects.
  • remove encryption param from tests which don't actually perform any object PUTs or MPU uploads (e.g. MantaClientDirectoriesIT)
  • change <packages> to <classes> and target specific classes. I was going to use test groups for this but it seems like TestNG has trouble with mixing group filters with packages/classes (e.g. the comment in testng-it.xml about how we can't use <groups><run><exclude>multipart</exclude></run></group>
  • document a new system property it.dryrun which can be used by the new interceptor. When this system property is truthy as defined by BooleanUtils#toBooleanObject, i.e. the following are all true: y, Y, t, T, on, ON, yes, YES, true, TRUE and all the mixed-case versions thereof
  • add a test listener called TestListingInterceptor which can be used to collect suite definitions and disable actually running tests in the event it.dryrun is truthy

Copy link
Contributor

@dekobon dekobon left a comment

Choose a reason for hiding this comment

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

LGTM

@cburroughs
Copy link
Contributor

Reminder: On the last run integration tests had 64% instruction coverage and 39% branch coverage. After this lands those numbers should stay the same.

@tjcelaya
Copy link
Contributor Author

For the dry-run aspect of this PR see testng-team/testng#1503. Updating to TestNG version 6.14 and using the -Dtestng.mode.dryrun=true option can be used to perform the test listing.

@indianwhocodes
Copy link
Contributor

This PR has been re-introduced as a different PR #504 since the changes introduced in #364 cannot be holistically integrated into the current SDK code.

@Parameters({"usingEncryption"})
public void beforeClass(@org.testng.annotations.Optional Boolean usingEncryption) throws IOException {
@Parameters({"encryptionCipher"})
public EncryptedServerSideMultipartManagerSerializationIT(final @Optional String encryptionCipher) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

A compilation-error will be generated here since this integration-test will mandatorily require invocation of the TestNG parameter encryptionCipher

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.

testng encryptionCipher parameter does not appear to do anything
4 participants