-
Notifications
You must be signed in to change notification settings - Fork 26
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
Added New Test Definitions error, seekable, etc. Remove redundant test runs & Introduce dry test runs #507
Added New Test Definitions error, seekable, etc. Remove redundant test runs & Introduce dry test runs #507
Conversation
Drive by color opinion: "nightly" isn't a term exclusive to Joyent, but still pretty far on the jargon scale. Is there a more self-descriptive term we could use? (slow? expensive?) |
|
I have not used testng groups before and am not sure if I understand how the flags are to be used. With |
Its because the |
When running the test from Maven using
|
java-manta-it/src/test/java/com/joyent/manta/client/MantaClientMetadataIT.java
Outdated
Show resolved
Hide resolved
...anta-it/src/test/java/com/joyent/manta/client/multipart/EncryptedJobsMultipartManagerIT.java
Show resolved
Hide resolved
...t/src/test/java/com/joyent/manta/client/multipart/EncryptedServerSideMultipartManagerIT.java
Outdated
Show resolved
Hide resolved
...va/com/joyent/manta/client/multipart/EncryptedServerSideMultipartManagerSerializationIT.java
Outdated
Show resolved
Hide resolved
...va/com/joyent/manta/client/multipart/EncryptedServerSideMultipartManagerSerializationIT.java
Outdated
Show resolved
Hide resolved
java-manta-it/src/test/java/com/joyent/manta/client/MantaClientMetadataIT.java
Outdated
Show resolved
Hide resolved
Also, I believe the comment here might be useful. |
|
I have -- I think -- the latest branch checked out. Git tells me the last commit is 4d6d73b. Do I have that right? When I do
But when I run mvn verify -Dit.dryRun=true -DexcludedGroups=expensive
Do these counts match yours? Shouldn't the number of expected tests go down when any |
Yes, These counts match mine, but when you invoke - |
@cburroughs the discussion here will shed more light on what I am talking about. Also,
|
Glad to hear I'm not crazy about the counts. Per our call, we have two use cases we want to be easy to from the command line (for jenkins). Anything fancier is okay as a convenience for developers and their IDEs: With this PR the answer to (1) is just the default from running |
I just noticed that Additionally, when running |
When running
|
Goals & Challenges updated in the PR Description along with additional changes in javadocs and testing documentation. |
I'm afraid I'm still having trouble getting the two uses case to work, this gives me 250 tests:
But neither of these are giving me > 250 tests
What is the set of flags to run all of the tests as we would for post-commit CI? |
So we are entirely removing the approach of having an All integration-tests that have been anointed as |
We would just be running these tests like we run normal integration-tests. For instance, |
In that case, is it still necessary to switch from wildcards to a explicit list in the testng suite file? I thought we did that as part of trying to get groups to work, but it has an ongoing cost (future engineers will inevitably mess it up) |
Yes but I explicitly added that in the documentation here that developers will have to add definitions to the testing suite once they come up with a new integration-test. I will make sure it gets added to the general SDK User Guide as well. Also, class-level definitions were in-place to reduce the redundancies significantly otherwise we were running each integration-test almost 6 times probably more. |
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.
I noted a few minor doc or formatting nits on the last read through. This is good to land once those are folded in. Thanks for shepherding this big set of changes through.
java-manta-it/src/test/java/com/joyent/manta/config/IntegrationTestConfigContext.java
Outdated
Show resolved
Hide resolved
java-manta-it/src/test/java/com/joyent/test/util/TestListingInterceptor.java
Outdated
Show resolved
Hide resolved
Thanks @cburroughs for this thorough review, learned a lot of much needed conventions, basics through this entire process. |
I ran the integration tests for the latest bits and found the following test failure:
|
It seems this particular failure occurred due to the |
I'm going to go ahead and approve the PR. Can you make another ticket to capture the problems with that particular test? |
Yes, Issue #542 created to actively track this issue. |
This branch has been squashed and merged into |
Goals
609
tests ->250
tests.java-manta-pre-it
or simply update the existing onejava-manta-it
for new PR's made against theJava SDK
.Challenges
Unfortunately TestNG doesn't comprehensively support filtering a test suite by groups so we had to be more explicit about suite definition.
Overview
Adds a
it.dryRun
property that can be used to list the tests that would run with their respective TestNG params. TestNG unfortunately doesn't completely support filtering a test suite by groups so thorough detailing was required in suite definition.Output of
mvn verify it.dryRun=true
using currenttestng-it.xml
used withTestListingInterceptor
:List for the output for before & after
-Dit.dryRun=true
respectively . Note thatdry-run-after
list is accurate since it shows single value forencryptionCipher
i.eAES128/CTR/NoPadding
. Other testing groups includecbc-cipher
andgcm-cipher
are also defined.Changes
change
usingEncryption
(boolean) to justencryptionCipher
(string). omission (null) disables encryptionadd a new constructor for
IntegrationTestConfigContext
which handles the aboveswitch from
@BeforeClass
with@Parameters
to test constructors with just@Parameters
. WheneverSkipException
was thrown orputDirectory
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.document a new system property
it.dryRun
which can be used by the new interceptor. When this system property is truthy as defined byBooleanUtils#toBooleanObject
, i.e. the following are all true:y
,Y
,t
,T
,on
,ON
,yes
,YES
,true
,TRUE
and all the mixed-case versions thereofadded a test listener called
TestListingInterceptor
which is used to collect suite definitions and disable actually running tests in the eventit.dryrun
istrue
.defined new
<test name></test>
and the EXISTING ones intesting-it.xml
more accurately using class-level specifications including TestNG Group Definitions :timeout
,range-downloads
,snaplinks
,seekable
,error
,headers
,directory
,metadata
, etc.Following up on Fixing Issue 483 For Defining HeavyWeight Test Groups #492 which had to be closed due to
dependency-vulnerabilities
.Changes are incorporated from the closed PR Remove redundant integration test runs #364 by Tomas which is to remove redundancies in
integration-test
runs.