You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/01/27 17:25:47 UTC
[GitHub] [accumulo] DomGarguilo opened a new pull request #2435: Convert all tests to assertThrows
DomGarguilo opened a new pull request #2435:
URL: https://github.com/apache/accumulo/pull/2435
The goal of this PR is to convert all usage of the older `@Test(expected = ...` form of exception testing to the newer `assertThrows()`.
An example conversion:
```java
@Test(expected = IllegalArgumentException.class)
public void testNegativeTimeout() {
BatchWriterConfig bwConfig = new BatchWriterConfig();
bwConfig.setTimeout(-1, TimeUnit.DAYS);
}
```
Would become:
```java
@Test
public void testNegativeTimeout() {
BatchWriterConfig bwConfig = new BatchWriterConfig();
assertThrows(IllegalArgumentException.class, () -> bwConfig.setTimeout(-1, TimeUnit.DAYS));
}
```
I used this command to verify all tests that were changed still pass:
`mvn clean verify -Dit.test=ConditionalWriterIT,ReadWriteIT,KerberosIT,MetaConstraintRetryIT,ShellServerIT,BulkNewIT,ZooKeeperPropertiesIT,AccumuloInputFormatIT,NewTableConfigurationIT,MetaSplitIT,CloneTestIT -Dtest=ZooReaderWriterTest,TabletMetadataTest,TCredentialsUpdatingInvocationHandlerTest,AuthorizationsTest,FastFormatTest,ReplicationProcessorTest,TabletLocationStateTest,RelativeKeyTest,InMemoryMapTest,GarbageCollectionTest,OpTimerTest,ConfigCheckUtilTest,ScannerOptionsTest,ConfigurationTypeHelperTest,LogFileKeyTest,DefaultFormatterTest,SortedMapIteratorTest,TabletServerSyncCheckTest,LoadPlanTest,MetadataTimeTest,Upgrader9to10Test,ZooAuthenticationKeyDistributorTest,AuthenticationTokenSecretManagerTest,MutationTest,AccumuloConfigurationTest,ArrayByteSequenceTest,ListLexicoderTest,HadoopCredentialProviderTest,WholeRowIteratorTest,ConditionTest,ProblemReportingIteratorTest,UnsynchronizedBufferTest,TabletTimeTest,ManagerReplicationCoordinatorTest,IteratorSettingTest,TabletMutat
ionPrepAttemptTest,ZooSessionTest,ValueTest,TServerUtilsTest,SpaceAwareVolumeChooserTest,RFileTest,BatchWriterConfigTest,LinkingIteratorTest,HexFormatterTest,RFileClientTest,CredentialProviderTokenTest,PasswordConverterTest,ReplicationSchemaTest,ServerContextTest,DelegationTokenConfigTest,KeyTest,ThriftTransportKeyTest,SequenceLexicoderTest,AccumuloTest,AuthenticationKeyTest,RowEncodingIteratorTest,ShellUtilTest,ZooKeeperInstanceTest -Dspotbugs.skip -Dtimeout.factor=1`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2435: Convert all tests to assertThrows
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2435:
URL: https://github.com/apache/accumulo/pull/2435#issuecomment-1023502551
@DomGarguilo I would prefer general code improvements like this that touch a lot of files to be merged in the next version (most likely 2.2) so to not slow down the 2.1 release. Did you have a reason why this needs to be in 2.1?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2435: Convert all tests to assertThrows
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2435:
URL: https://github.com/apache/accumulo/pull/2435#issuecomment-1023526248
> > Did you have a reason why this needs to be in 2.1?
>
> Nope, it can wait. Do you think I should mark this as draft until then?
I was thinking we could create a temporary branch for 2.2 then merge it when we release 2.1. @ctubbsii any thoughts on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2435: Convert all tests to assertThrows
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2435:
URL: https://github.com/apache/accumulo/pull/2435#issuecomment-1023622108
This can be merged. I talked to Ed and he is OK with the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on pull request #2435: Convert all tests to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2435:
URL: https://github.com/apache/accumulo/pull/2435#issuecomment-1023511365
> Did you have a reason why this needs to be in 2.1?
Nope, it can wait. Do you think I should mark this as draft until then?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2435: Convert all tests to assertThrows
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2435:
URL: https://github.com/apache/accumulo/pull/2435#issuecomment-1023589176
> @DomGarguilo I would prefer general code improvements like this that touch a lot of files to be merged in the next version (most likely 2.2) so to not slow down the 2.1 release. Did you have a reason why this needs to be in 2.1?
These are all test files, with a very predictable fix, that I need to get merged for my subsequent fixes. I don't think there's a reason to delay on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on pull request #2435: Convert all tests to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2435:
URL: https://github.com/apache/accumulo/pull/2435#issuecomment-1023619000
Either merging now or waiting until after 2.1.0 is released is fine with me. What do you think, @milleruntime?
This change (#2435), which can stand alone, is a precursor to a set of other changes I have in mind that relate to upgrading JUnit from version 4 to 5. Those future changes, which may look similar to those in #2427, may be more disruptive and I am assuming we would want to hold off on a lot of those changes until after the 2.1.0 release. This PR might be fine to merge but for changes like that, should we just hold off on working on those altogether or should there be a separate branch for them like @milleruntime suggested?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2435: Convert all tests to assertThrows
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2435:
URL: https://github.com/apache/accumulo/pull/2435#issuecomment-1023635315
My followup changes to this are in #2436
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo merged pull request #2435: Convert all tests to assertThrows
Posted by GitBox <gi...@apache.org>.
DomGarguilo merged pull request #2435:
URL: https://github.com/apache/accumulo/pull/2435
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org