DRILL-5751: Fix unit tests to use local file system even if it is not… #927
Conversation
| @@ -120,8 +132,11 @@ public void testDisableDynamicSupport() throws Exception { | |||
| @Test | |||
| public void testAbsentBinaryInStaging() throws Exception { | |||
| Path staging = getDrillbitContext().getRemoteFunctionRegistry().getStagingArea(); | |||
| FileSystem fs = getDrillbitContext().getRemoteFunctionRegistry().getFs(); | |||
| copyJar(fs, jars, staging, default_binary_name); | |||
There was a problem hiding this comment.
It seems that a source file should be copied here instead of the binary file, since test checks that binary file is missing.
There was a problem hiding this comment.
Good catch! Fixed.
| fs = FileSystem.get(new Configuration()); | ||
| Configuration configuration = new Configuration(); | ||
| configuration.set(FileSystem.FS_DEFAULT_NAME_KEY, FileSystem.DEFAULT_FS); | ||
| fs = FileSystem.get(configuration); |
There was a problem hiding this comment.
These three lines appear several times in the code, so may be it would be better to create a static method which returns FileSystem instance.
There was a problem hiding this comment.
Agree. Added static method ExecTest.getLocalFileSystem.
arina-ielchiieva
force-pushed
the
DRILL-5751
branch
from
079aa8a
to
3a27bc7
Compare
There was a problem hiding this comment.
@vvysotskyi thanks for code review. Addressed code review comments in new commit.
| @@ -120,8 +132,11 @@ public void testDisableDynamicSupport() throws Exception { | |||
| @Test | |||
| public void testAbsentBinaryInStaging() throws Exception { | |||
| Path staging = getDrillbitContext().getRemoteFunctionRegistry().getStagingArea(); | |||
| FileSystem fs = getDrillbitContext().getRemoteFunctionRegistry().getFs(); | |||
| copyJar(fs, jars, staging, default_binary_name); | |||
There was a problem hiding this comment.
Good catch! Fixed.
| fs = FileSystem.get(new Configuration()); | ||
| Configuration configuration = new Configuration(); | ||
| configuration.set(FileSystem.FS_DEFAULT_NAME_KEY, FileSystem.DEFAULT_FS); | ||
| fs = FileSystem.get(configuration); |
There was a problem hiding this comment.
Agree. Added static method ExecTest.getLocalFileSystem.
|
LGTM! +1. |
… set by default DRILL-5751: Changes after code review. close apache#927
|
+1 |
… set by default
Please refer to DRILL-5751 for details.