-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Support parameterized tests at class-level with JUnit5 #9161
Conversation
I wonder if this workaround is good enough. Maybe we need to discuss a plan to deal with this situation. @nastra Could you like to help us by providing some suggestions? |
@RunWith(Parameterized.class) | ||
public class TestDictionaryRowGroupFilter { | ||
/** | ||
* At time of development there's no out of the box way to implement a Parameterized BeforeEach in Junit5. |
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 don't think we actually want to create multiple classes for each parameter. I think it would make sense to check how easy/complicated it is to support class-level parameterization with JUnit5 as I know that's not supported by default
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.
maybe worth exploring whether junit-team/junit5#3157 (comment) would fit here by using a CustomParameterResolver
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.
Hi @lisirrx @nastra , I met the same problem when I tried to solve #9079. Can we refer to Flink here, introduce a ParameterizedTestExtension?
If you agree with me, I'd like to create a PR to do this.
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.
@wanglijie95 @nastra, ParameterizedTestExtension introduced in 4d381f8.
I put it in an util test package, but I think the best idea should be to move these utils in a shared module. Is it better to open a new issue to address it?
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 personally think it is. Put it into a shared module so that it can be used in multiple modules. Besides, I think it's better to add a reference in the ParameterizedTestExtension's doc to indicate it was inspired from Apache Flink. WDYT?
I'd also like to hear @nastra 's thoughts.
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.
+1, I think a shared module is better.
BTW, I will check if this approach fits the iceberg-core
module.
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.
CC: @cgpoh, you might be interested in this
public void createInputFile() throws IOException { | ||
File parquetFile = temp.newFile(); |
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 encounter the same case in iceberg-core
. I step into the newFile
function and find it create a temp file with prefix junit
, using File.createTempFile
. May be we can use the File.createTempFile
directly in our code here?
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.
please check other code that was migrated to JUnit5 to see how this was handled
@@ -18,37 +18,13 @@ | |||
*/ | |||
package org.apache.iceberg.parquet; | |||
|
|||
import static org.apache.iceberg.avro.AvroSchemaUtil.convert; |
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.
these shouldn't change
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.
Solved in 833e9bf.
@GianlucaPrincipini can you please run |
* | ||
* <p>When use this extension, all tests must be annotated by {@link TestTemplate}. | ||
*/ | ||
public class ParameterizedTestExtension implements TestTemplateInvocationContextProvider { |
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.
this should probably go into the tests under iceberg-api
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.
Solved in 833e9bf
} catch (Exception e) { | ||
throw new IllegalStateException("Failed to invoke parameter provider", e); | ||
} | ||
assert parameterValues != null; |
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.
this should be Preconditions.checkState(..)
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.
Solved in 653fec1
import org.junit.runners.Parameterized; | ||
|
||
@RunWith(Parameterized.class) | ||
import org.junit.jupiter.api.Assumptions; |
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.
please use the AssertJ assumtions
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.
Solved in 833e9bf
} | ||
|
||
@TempDir | ||
public Path temp; |
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.
can be private
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.
Solved in 6bfb542
@GianlucaPrincipini I would probably rename this PR so that it directly addresses #9210 (because that's what it is doing here). We might also want to convert a few more parameterized tests to using this, just to make sure we're not missing anything |
@nastra, sounds ok to me. I will work on migrating some other class in the next few days |
* @param containedInMessage A String that should be contained by the thrown exception's message | ||
* @param callable A Callable that is expected to throw the exception | ||
*/ | ||
public static void assertThrows( |
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.
why is this being added here? It looks like this is from AssertHelpers
, which is deprecated. That being said, please revert changes against this file
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.
api/src/test/java/org/apache/iceberg/ParameterizedTestExtension.java
Outdated
Show resolved
Hide resolved
import java.util.List; | ||
import java.util.function.Function; | ||
import java.util.stream.Stream; | ||
import org.assertj.core.util.Preconditions; |
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.
this is actually the wrong import. Should be org.apache.iceberg.relocated.com.google.common.base.Preconditions
public GenericAppenderHelper( | ||
Table table, FileFormat fileFormat, TemporaryFolder tmp, Configuration conf) { | ||
Table table, FileFormat fileFormat, TemporaryFolder tmp, Configuration conf) |
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.
let's roll back these changes and focus only on adding the ParameterizedTestExtension
and converting TestDictRowGroupFilter
@GianlucaPrincipini this PR now contains unfortunately more changes than I anticipated, so I would suggest to focus only on adding the Make sure to run |
…n.java Co-authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>
@GianlucaPrincipini the PR still contains more changes than necessary. The scope of this PR is to add the parameterized test extensions and the conversion of |
parquet/src/test/java/org/apache/iceberg/parquet/TestDictionaryRowGroupFilter.java
Outdated
Show resolved
Hide resolved
NullPointerException.class, | ||
"Cannot create expression literal from null", | ||
() -> notStartsWith("col", null)); | ||
Assertions.assertThatThrownBy(() -> equal("col", null)) |
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.
nit: Could we do a static import for this?
Feature Request / Improvement
Closes #9210
The goal is to switch all imports to JUni5 imports and to use AssertJ-style assertions.
Since in Junit5 there is no way to parameterize a full test class yet (junit-team/junit5#878),
ParameterizedTestExtension
extension class andParameters
/Parameter
have been introduced.