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

JUnit5 extension classes #28

Merged
merged 18 commits into from
Sep 2, 2019
Merged

JUnit5 extension classes #28

merged 18 commits into from
Sep 2, 2019

Conversation

cmathiesen
Copy link
Contributor

@cmathiesen cmathiesen commented Aug 12, 2019

Fixes issue #22.
I've created some extension classes for each of the JUnit4 rules, that matches the same structure as the rules, and use the @RegisterExtension annotation. An upgrade to Java 8 was necessary to use JUnit5.

@cmathiesen cmathiesen requested review from massdosage and patduin and removed request for massdosage August 12, 2019 13:35
init();
beforeTest();
} catch (Throwable throwable) {
LOG.error("Unable to initialise BeejuJUnit extension", throwable);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really useful, imagine running a test that is not working and you only have this this error in the logs somewhere., that's hard to debug. Just remove the try catch and let all the exceptions bubble up. Fail fast

afterTest();
try {
FileUtils.deleteDirectory(tempDirectory.toFile());
} catch (IOException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here (also never printStacktrace please).

* "--url",
* hive.connectionURL(),
* "--user",
* HiveMetaStoreJUnitRule.HSQLDB_USER,
Copy link
Contributor

Choose a reason for hiding this comment

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

could you test if this still works, I think we are using Derby DB so those USER/PASSWORD things are wrong. We definitely shouldn't refer to the Rule here.

*
* @throws Throwable If the initialisation fails.
*/
protected void init() throws Throwable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe throws Exception (that's eventually what the beforeEach does as well)

import com.hotels.beeju.core.ThriftHiveMetaStoreCore;

/**
* A JUnit Extension that creates a Hive Metastore Thrift service backed by a Hive Metastore using an HSQLDB in-memory
Copy link
Contributor

Choose a reason for hiding this comment

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

we're not using HSQLDB but Derby: can please change it to in-memory database no reason to mention the type. I think I neglected to update this when we moved to Derby, sorry :(

}

@Test
public void addPartition() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is lots of duplication in between this class and the com.hotels.beeju.HiveServer2JUnitRuleTest. Perhaps some of those tests can move to com.hotels.beeju.core.HiveServer2CoreTest and the Rule/Extension test can test just the logic of the Rule/Extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I had a similar thought when looking at it. But I think the issue with doing that is these tests all use different elements of the rule/extension which isn't available in the HiveServer2Core, such as the temporary directory and the server connection URL. Maybe some of the reused logic could be moved into a test resources class?

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 it's better to move it to a core test as it is core logic we're testing. The core test should just provide a temporary directory and a server connection url.

@coveralls
Copy link

coveralls commented Aug 14, 2019

Coverage Status

Coverage decreased (-21.1%) to 69.675% when pulling f4c7901 on junit5-extensions into 6bfa56d on master.

@cmathiesen cmathiesen marked this pull request as ready for review August 16, 2019 10:33
@cmathiesen cmathiesen requested a review from AnanaMJ August 16, 2019 10:34
README.md Outdated
This rule creates an in-memory Hive database and a Thrift Hive Metastore service on top of this. This can then be used to perform Hive Thrift API calls in a test. The rule exposes a Thrift URI that can be injected into the class under test and a Hive Metastore Client which can be used for data setup and assertions.

Example usage: Class under test creates a table via the Hive Metastore Thrift API.
Example @Rule usage: Class under test creates a table via the Hive Metastore Thrift API.
Copy link
Contributor

Choose a reason for hiding this comment

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

@Rule would look good wrapped in back ticks :)

README.md Outdated
This rule creates an in-memory Hive database without a Thrift Hive Metastore service. This can then be used to perform Hive API calls directly (i.e. without going via Hive's Metastore Thrift service) in a test.

Example usage: Class under test creates a partition using an injected Hive Metastore Client.
Example @ Rule usage: Class under test creates a partition using an injected Hive Metastore Client.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor

Choose a reason for hiding this comment

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

and there is a space between @ and Rule

import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;

import com.hotels.beeju.core.HiveMetaStoreCore;

/**
* A JUnit {@link Rule} that creates a Hive Metastore backed by an HSQLDB in-memory database.
Copy link
Contributor

Choose a reason for hiding this comment

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

This showed in red for me in IntelliJ. But after adding an import for org.junit.Rule, it looked fine. And there are 2 other classes that have javadocs rules for unimported linked classes. It looks like these are unresolved references inside the Javadoc. Do we care about these or not?

* This method can be overridden to provide additional initialisations.
* </p>
*
* @throws Throwable If the initialisation fails.
Copy link
Contributor

Choose a reason for hiding this comment

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

Small thing, but it should be lowercase "i" in "If". And it throws IOException, not Throwable, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh it looks like all the javadocs with @throws Exception ... starts with uppercased "i" in "If" so nevermind about that. But I still think the exception should be IOException :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep good spot!

}

/**
* @return @see com.hotels.beeju.core.BeejuCore#connectionURL()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be @link instead of @see?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it should :)


@Test
public void createDatabaseNullName() {
Assertions.assertThrows(NullPointerException.class, () -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be on one line:

Assertions.assertThrows(NullPointerException.class, () -> hiveDefaultName.createDatabase(null));


@Test
public void defaultDatabaseName() {
HiveServer2JUnitExtension server = new HiveServer2JUnitExtension(DATABASE, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed here? Because the test passes without it too.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 it's unused


@Test
public void createExistingDatabase() {
Assertions.assertThrows(AlreadyExistsException.class, () -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

The lambdas from here and the next two tests can also be on one line each :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe except for this one, because it doesn't fit on one line, so it would be something like this

    Assertions.assertThrows(AlreadyExistsException.class,
        () -> hiveDefaultName.createDatabase(hiveDefaultName.databaseName()));

@Test
public void customProperties() {
Map<String, String> conf = new HashMap<>();
conf.put("my.custom.key", "my.custom.value");
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want this to be on one line, I found out recently that you can do

Map<String, String> conf = Collections.singletonMap("my.custom.key", "my.custom.value");


Database database = hive.client().getDatabase(databaseName);

assertThat(database.getName(), is(databaseName));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is more of a personal preference, but I wouldn't split the declaration of databaseName and of database from the assert for the database names. To me they should be together because they're about the same thing.

I would also add a new line between line 81 and 83.

Like I said, this is personal preference, so take it or leave it 🙂

pom.xml Outdated
<dependency>
<groupId>hsqldb</groupId>
<artifactId>hsqldb</artifactId>
<version>1.8.0.1</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is for the javadoc right? <scope>provided</scope> should be ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, but seeing as we took it out of the JavaDoc i'll just get rid of it here

Copy link
Contributor

Choose a reason for hiding this comment

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

even better

src/main/java/com/hotels/beeju/core/BeejuCore.java Outdated Show resolved Hide resolved

public void deleteTempDir() {
try {
Files.walkFileTree(tempDir, new SimpleFileVisitor<Path>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

commons IO solves this for you: FileUtils.deleteDirectory(tempDir);

Copy link
Contributor Author

@cmathiesen cmathiesen Aug 19, 2019

Choose a reason for hiding this comment

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

I've added this instead which is much neater, however this still needs to handle an IOException but the base class that we are extending BeejuJUnitRule from doesn't let you throw exceptions in the signature of after() (which calls this deleteTempDir() method) so it has to be caught in a try/catch - is there a better method of handling it than printing the stack trace?

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 this should still just throw cause it can't handle it. In the rule it can try catch(){throw new runtimeException(e);} and I think in the extension where it is called it will just be thrown no need to do anything there.

}
});
} catch (IOException e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

Never printStacktrace please, that's pretty much the same as ignoring the exception altogether.
Just throw the exception in the method signature

src/main/java/com/hotels/beeju/core/BeejuCore.java Outdated Show resolved Hide resolved
@@ -75,11 +87,27 @@ public BeejuCore(String databaseName, Map<String, String> configuration) {
// overriding default derby log path to go to tmp
String derbyLog = File.createTempFile("derby", ".log").getCanonicalPath();
System.setProperty("derby.stream.error.file", derbyLog);

//Creating temporary folder
init();
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 either have an external class call init() and don't init in the constructor or leave the init() call here and make the method private? Nothing is overwriting it atm so it can be private

Copy link
Contributor

Choose a reason for hiding this comment

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

if private also please rename to something like: createWarehousePath()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i'll make this private and change the name, this part should happen during core initialisation to have access to the temporary directory. Although it is only 2 lines, which could be added to the constructor instead?

src/main/java/com/hotels/beeju/BeejuJUnitRule.java Outdated Show resolved Hide resolved
*
* @throws Exception If the extension cannot be prepared for a new test.
*/
protected abstract void beforeTest() throws Exception;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this method, can't we just override beforeEach in the subclasses?

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 guess technically no we dont need it, but adding beforeTest() as an abstract method means we keep the logic of calling BeejuCore methods here in BeejuJUnitExtension (init() and createDatabase()) and then handle calling the HiveMetaStoreCore (or other variation) methods (initialise()) in the corresponding Extension class which seems like a more logical way of structuring it to me

* This method is called before the warehouse directory is deleted.
* </p>
*/
protected abstract void afterTest() throws Exception;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this method, can't we just override afterEach in the subclasses?


@Test
public void defaultDatabaseName() {
HiveServer2JUnitExtension server = new HiveServer2JUnitExtension(DATABASE, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 it's unused

Copy link
Member

@abhimanyugupta07 abhimanyugupta07 left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me.

Copy link
Contributor

@patduin patduin left a comment

Choose a reason for hiding this comment

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

looks good

import java.util.Map;

import org.apache.hadoop.hive.conf.HiveConf;
import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
import org.apache.thrift.TException;
import org.junit.rules.ExternalResource;
import org.junit.rules.TemporaryFolder;

import com.google.common.annotations.VisibleForTesting;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how this import stayed here because the save actions should've removed it, but it should be removed :)

import java.nio.file.FileVisitResult;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.SimpleFileVisitor;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, there are some imports that aren't needed.


public class BeejuCore {

private static final Logger LOG = LoggerFactory.getLogger(BeejuCore.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't used

@Test
public void createExistingDatabase() {
Assertions.assertThrows(AlreadyExistsException.class, ()
-> {
Copy link
Contributor

Choose a reason for hiding this comment

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

what happened here? XD

Copy link
Contributor

Choose a reason for hiding this comment

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

(() -> hiveDefaultName.createDatabase(hiveDefaultName.databaseName())); could be on this line)

Assertions.assertThrows(AlreadyExistsException.class, () -> {
hiveDefaultName.createDatabase(hiveDefaultName.databaseName());
});
Assertions.assertThrows(AlreadyExistsException.class, () -> hiveDefaultName.createDatabase(hiveDefaultName.databaseName()));
Copy link
Contributor

Choose a reason for hiding this comment

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

this is probably supposed to be on 2 lines (over 120 characters :( )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are correct :')

pom.xml Show resolved Hide resolved
README.md Outdated

[![Maven Central](https://maven-badges.herokuapp.com/maven-central/com.hotels/beeju/badge.svg?subject=com.hotels:beeju)](https://maven-badges.herokuapp.com/maven-central/com.hotels/beeju) [![Build Status](https://travis-ci.org/HotelsDotCom/beeju.svg?branch=master)](https://travis-ci.org/HotelsDotCom/beeju) [![Coverage Status](https://coveralls.io/repos/github/HotelsDotCom/beeju/badge.svg?branch=master)](https://coveralls.io/github/HotelsDotCom/beeju) ![GitHub license](https://img.shields.io/github/license/HotelsDotCom/beeju.svg)

# Overview
BeeJU provides [JUnit rules](http://junit.org/junit4/javadoc/4.12/org/junit/Rule.html) that can be used to write test code that tests [Hive](https://hive.apache.org/). A JUnit rule is a means to provide resources in a test and automatically tear them down when the life cycle of a test ends.
BeeJU provides [JUnit4 rules](http://junit.org/junit4/javadoc/4.12/org/junit/Rule.html) that can be used to write test code that tests [Hive](https://hive.apache.org/). A JUnit rule is a means to provide resources in a test and automatically tear them down when the life cycle of a test ends.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's switch this around and call out JUnit5 first as this is the preferred way going forward. So.. "BeeJU provides JUnit5 Extensions... etc." and then "BeeJU also provides JUnit4 rules" etc.

README.md Outdated

### JUnit5
For JUnit5, support is available to enable you to migrate your tests that currently use Beeju rules. To use JUnit5, ensure you have:
### JUnit5 Rule Migration
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe redo this and have JUnit5 documented first with just examples of the extensions and then a separate section afterwards for JUnit4 which just has the rules. I know this has more repeated code but for readers they can just look at one section and don't have to think about whether they're doing a rule or an extension.

pom.xml Show resolved Hide resolved
src/main/java/com/hotels/beeju/BeejuJUnitRule.java Outdated Show resolved Hide resolved

public class ThriftHiveMetaStoreJUnitExtensionTest implements BeforeEachCallback {

private static File defaultTempRoot;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for these two being static? If not we generally favour not using static (for various reasons we can go into).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The temp folders aren't actually needed anymore for these tests either so I'll remove them, but happy to learn more about static vs non-static :D

README.md Outdated
BeeJU provides [JUnit rules](http://junit.org/junit4/javadoc/4.12/org/junit/Rule.html) that can be used to write test code that tests [Hive](https://hive.apache.org/). A JUnit rule is a means to provide resources in a test and automatically tear them down when the life cycle of a test ends.
This project is currently built with and tested against Hive 2.3.0 (and minor versions back to Hive 1.2.1) but is most likely compatible with older and newer versions of Hive. The available JUnit rules are explained in more detail below.
BeeJU provides [JUnit5 Extensions](https://junit.org/junit5/docs/current/user-guide/#extensions) that can be used to write test code that tests [Hive](https://hive.apache.org/). The JUnit lifecycle extension points are a means to provide resources in a test and automatically tear them down when the life cycle of a test ends.
This project is currently built with and tested against Hive 2.3.0 (and minor versions back to Hive 1.2.1) but is most likely compatible with older and newer versions of Hive. The available JUnit extensions are explained in more detail below.
Copy link
Contributor

Choose a reason for hiding this comment

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

2.3.0 -> 2.3.x
(not your fault this out of date but might as well fix it now)

CHANGELOG.md Outdated
- JDK version upgrade to 1.8 (was 1.7).

### Added
- JUnit5 extension class equivalents for all Beeju Rules.
Copy link
Contributor

Choose a reason for hiding this comment

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

Beeju -> BeeJU
(can you do find and replace on this in the while file plz)

README.md Outdated
# Usage
The BeeJU JUnit rules provide a way to run tests that have an underlying requirement to use the Hive Metastore API but don't have the ability to mock the [Hive Metastore Client](https://hive.apache.org/javadocs/r1.2.1/api/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.html). The rules spin up and tear down an in-memory Metastore which may add few seconds to the test life cycle so if you require tests to run in the sub-second range this is not for you.
The BeeJU JUnit rules and extensions provide a way to run tests that have an underlying requirement to use the Hive Metastore API but don't have the ability to mock the [Hive Metastore Client](https://hive.apache.org/javadocs/r1.2.1/api/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.html). The rules and extensions spin up and tear down an in-memory Metastore which may add a few seconds to the test life cycle so if you require tests to run in the sub-second range this is not for you.
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're fixing the outdated things, can you please update the javadoc link to https://hive.apache.org/javadocs/r2.3.6/api/index.html (I couldn't find javadoc for 2.3.4).

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/main/java/com/hotels/beeju/BeejuJUnitRule.java Outdated Show resolved Hide resolved
src/main/java/com/hotels/beeju/core/BeejuCore.java Outdated Show resolved Hide resolved

import com.hotels.beeju.core.BeejuCore;

abstract public class BeejuJUnitExtension implements BeforeEachCallback, AfterEachCallback {
Copy link
Contributor

Choose a reason for hiding this comment

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

switch public and abstract around (that's our convention to have access level first, granted we don't document this anywhere so it needs to be filed under "tribal knowledge")


abstract public class BeejuJUnitExtension implements BeforeEachCallback, AfterEachCallback {

public BeejuCore core;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this be made private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the 3 classes we extend from BeejuJUnitExtension we have to pass the core object we've created into the next layer core class e.g. to create the HiveServer2Core object you need the BeejuCore object that we create here so I don't think this can be private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But i shall make it protected!

@massdosage massdosage merged commit 28f4656 into master Sep 2, 2019
@massdosage massdosage deleted the junit5-extensions branch September 2, 2019 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants