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.

Sign up for GitHub

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

Jump to bottom

Migrate TestDerbyConnector to a JUnit @Rule #1672

Merged
merged 1 commit into from
Aug 27, 2015
Merged

Migrate TestDerbyConnector to a JUnit @Rule #1672

merged 1 commit into from
Aug 27, 2015

Conversation

drcrallen
Copy link
Contributor

  • Adds UUID to every test's DB name as well as table space (for default behavior)
    Hopefully this reduces the irregular test failures.

@drcrallen
Copy link
Contributor Author

First run is green. Firing off again to see if it holds.

@drcrallen drcrallen closed this Aug 26, 2015
@drcrallen drcrallen reopened this Aug 26, 2015
@drcrallen
Copy link
Contributor Author

Fire 2 is green, going for a trifecta

@drcrallen drcrallen closed this Aug 26, 2015
@drcrallen drcrallen reopened this Aug 26, 2015
@drcrallen
Copy link
Contributor Author

3 for 3 greens. Looking good

xvrl
xvrl reviewed Aug 26, 2015
View reviewed changes
server/src/test/java/io/druid/metadata/TestDerbyConnector.java

public DerbyConnectorRule()
{
this(Suppliers.ofInstance(MetadataStorageTablesConfig.fromBase("druidTest" + dbSafeUUID())));
Copy link
Member

Choose a reason for hiding this comment

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

is it necessary to create unique table IDs, if we already create a unique database per test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Necessary? no. I put it in as a safeguard in case someone re-uses either this code or this class for other stuff later.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's good to be able to test table creation both with default names and with a specified prefix, so let's remove it if it's not necessary. Each test method already gets it's unique database, so that should be enough. It will make the test code simpler and less confusing.

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

@drcrallen drcrallen force-pushed the derbyRule branch 3 times, most recently from 65c6605 to 959465f Compare August 26, 2015 21:52
xvrl
xvrl reviewed Aug 26, 2015
View reviewed changes
indexing-hadoop/src/test/java/io/druid/indexer/updater/HadoopConverterJobTest.java
@@ -201,6 +118,20 @@ public void tearDown()
@Before
public void setUp() throws Exception
{
final MetadataStorageUpdaterJobSpec metadataStorageUpdaterJobSpc = new MetadataStorageUpdaterJobSpec()
Copy link
Member

Choose a reason for hiding this comment

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

typo in Spec

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

@drcrallen
Copy link
Contributor Author

Woohoo! 167 additions and 215 deletions removing lines

@xvrl
Copy link
Member

xvrl commented Aug 26, 2015

👍 when travis completes

himanshug
himanshug reviewed Aug 27, 2015
View reviewed changes
server/src/test/java/io/druid/metadata/TestDerbyConnector.java

public class TestDerbyConnector extends DerbyConnector
{
public static class DerbyConnectorRule extends ExternalResource
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor Nit: Usually internal classes are defined in the end rather than the first thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was unfamiliar with that convention. Fixed

@himanshug
Copy link
Contributor

aside from minor nit comment (can be ignored if you want) LGTM

@gianm
Copy link
Contributor

gianm commented Aug 27, 2015

👍 after travis

@gianm gianm closed this Aug 27, 2015
@gianm gianm reopened this Aug 27, 2015
@gianm
Copy link
Contributor

gianm commented Aug 27, 2015

travis bounce

@drcrallen
Copy link
Contributor Author

@gianm What test failed?

nvmnd, found it https://travis-ci.org/druid-io/druid/jobs/77442454

testDelete(io.druid.server.namespace.cache.NamespaceExtractionCacheManagerExecutorsTest)  Time elapsed: 50.008 sec  <<< ERROR!
java.lang.Exception: test timed out after 50000 milliseconds
    at io.druid.server.namespace.cache.NamespaceExtractionCacheManager.removeNamespaceLocalMetadata(NamespaceExtractionCacheManager.java:296)
    at io.druid.server.namespace.cache.NamespaceExtractionCacheManager.delete(NamespaceExtractionCacheManager.java:514)
    at io.druid.server.namespace.cache.OnHeapNamespaceExtractionCacheManager.delete(OnHeapNamespaceExtractionCacheManager.java:99)
    at io.druid.server.namespace.cache.NamespaceExtractionCacheManagerExecutorsTest.testDelete(NamespaceExtractionCacheManagerExecutorsTest.java:333)
    at io.druid.server.namespace.cache.NamespaceExtractionCacheManagerExecutorsTest.testDelete(NamespaceExtractionCacheManagerExecutorsTest.java:258)
Running io.druid.server.namespace.URIExtractionNamespaceFunctionFactoryTest
Tests run: 12, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 4.565 sec - in io.druid.server.namespace.URIExtractionNamespaceFunctionFactoryTest
Results :
Tests in error: 
  NamespaceExtractionCacheManagerExecutorsTest.testDelete:258->testDelete:333 » ```

@gianm
Copy link
Contributor

gianm commented Aug 27, 2015

@drcrallen, it was this job:

https://travis-ci.org/druid-io/druid/jobs/77442454

Tests in error:
NamespaceExtractionCacheManagerExecutorsTest.testDelete:258->testDelete:333 »

gianm added a commit that referenced this pull request Aug 27, 2015
@gianm
Merge pull request #1672 from metamx/derbyRule
5b67ec6 
Migrate TestDerbyConnector to a JUnit @rule
@gianm gianm merged commit 5b67ec6 into apache:master Aug 27, 2015
@drcrallen drcrallen deleted the derbyRule branch August 27, 2015 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@drcrallen @xvrl @himanshug @gianm