You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2022/12/07 15:12:38 UTC
[GitHub] [zeppelin] huage1994 opened a new pull request, #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
huage1994 opened a new pull request, #4532:
URL: https://github.com/apache/zeppelin/pull/4532
### What is this PR for?
This PR is to migrate the tests in `zeppelin-zengine` module from JUnit 4 to JUnit 5, which is a part of [JUnit 5 Migration Plan](https://issues.apache.org/jira/browse/ZEPPELIN-5851)
### What type of PR is it?
Improvement
Tests
### Todos
* [ ] - Task
### What is the Jira issue?
Open an issue on Jira https://issues.apache.org/jira/browse/ZEPPELIN-5851
### How should this be tested?
* CI passed.
### Screenshots (if appropriate)
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a diff in pull request #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
Posted by GitBox <gi...@apache.org>.
Reamer commented on code in PR #4532:
URL: https://github.com/apache/zeppelin/pull/4532#discussion_r1043276481
##########
zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTest.java:
##########
@@ -46,66 +46,66 @@ public void getAllowedOrigins2Test() throws MalformedURLException {
ZeppelinConfiguration conf = ZeppelinConfiguration.create("test-zeppelin-site2.xml");
List<String> origins = conf.getAllowedOrigins();
- Assert.assertEquals(2, origins.size());
- Assert.assertEquals("http://onehost:8080", origins.get(0));
- Assert.assertEquals("http://otherhost.com", origins.get(1));
+ Assertions.assertEquals(2, origins.size());
Review Comment:
Trivia. I would prefer just `assertEquals` without `Assert` or `Assertion`.
You have already imported assertEquals statically above.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
Posted by "huage1994 (via GitHub)" <gi...@apache.org>.
huage1994 commented on PR #4532:
URL: https://github.com/apache/zeppelin/pull/4532#issuecomment-1534337348
> @huage1994 Please rebase your current branch. I merged #4582 right now.
Done.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
Posted by "huage1994 (via GitHub)" <gi...@apache.org>.
huage1994 commented on PR #4532:
URL: https://github.com/apache/zeppelin/pull/4532#issuecomment-1400410386
> @huage1994 I think we can proceed with this change. Please rebase your branch to the current master.
Thanks @Reamer for reminding! 👍
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
Posted by "huage1994 (via GitHub)" <gi...@apache.org>.
huage1994 commented on PR #4532:
URL: https://github.com/apache/zeppelin/pull/4532#issuecomment-1512627023
There still seems to be a problem. I'm working on it.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on pull request #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on PR #4532:
URL: https://github.com/apache/zeppelin/pull/4532#issuecomment-1400278340
@huage1994
I think we can proceed with this change. Please rebase your branch to the current master.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4532:
URL: https://github.com/apache/zeppelin/pull/4532#discussion_r1044995362
##########
zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NoteManagerTest.java:
##########
@@ -21,10 +21,11 @@
import org.apache.zeppelin.notebook.exception.NotePathAlreadyExistsException;
import org.apache.zeppelin.notebook.repo.InMemoryNotebookRepo;
import org.apache.zeppelin.user.AuthenticationInfo;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.ExpectedException;
+
+
Review Comment:
nit: use a single line only to divide sections
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on pull request #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on PR #4532:
URL: https://github.com/apache/zeppelin/pull/4532#issuecomment-1508203655
Hi @huage1994,
I have opened https://github.com/huage1994/zeppelin/pull/2. These changes should ensure that the tests go through.
I have also removed `mockito-junit-jupiter`. This only required rewriting one test class.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
Posted by "huage1994 (via GitHub)" <gi...@apache.org>.
huage1994 commented on PR #4532:
URL: https://github.com/apache/zeppelin/pull/4532#issuecomment-1523121562
This PR would be ready for review after #4582 is merged.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
Posted by "huage1994 (via GitHub)" <gi...@apache.org>.
huage1994 commented on PR #4532:
URL: https://github.com/apache/zeppelin/pull/4532#issuecomment-1512535026
> Hi @huage1994, I have opened [huage1994#2](https://github.com/huage1994/zeppelin/pull/2). These changes should ensure that the tests go through. I have also removed `mockito-junit-jupiter`. This only required rewriting one test class.
Sorry for late reply. Thanks @Reamer a lot. I have merged it.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 commented on pull request #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
Posted by GitBox <gi...@apache.org>.
huage1994 commented on PR #4532:
URL: https://github.com/apache/zeppelin/pull/4532#issuecomment-1343912407
> Really great work. I only noted one small thing.
> The bigger problem is that currently core-modules are no longer passing in the CI system, so we need to fix the error that occurs there before we can merge this pull reques
Thanks for your review @Reamer. I totally agree with you. I'll take a look at the CI problem as well.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] huage1994 merged pull request #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
Posted by "huage1994 (via GitHub)" <gi...@apache.org>.
huage1994 merged PR #4532:
URL: https://github.com/apache/zeppelin/pull/4532
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4532:
URL: https://github.com/apache/zeppelin/pull/4532#discussion_r1044995483
##########
zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/ParagraphTextParserTest.java:
##########
@@ -136,38 +136,32 @@ public void testParagraphTextQuotedPropertyKeyAndValue() {
assertEquals("", parseResult.getScriptText());
}
- @Rule
- public ExpectedException exceptionRule = ExpectedException.none();
@Test
public void testParagraphTextUnfinishedConfig() {
- exceptionRule.expect(RuntimeException.class);
- exceptionRule.expectMessage(
+ assertThrows(RuntimeException.class, () ->
+ ParagraphTextParser.parse("%spark.pyspark(pool="),
"Problems by parsing paragraph. Not finished interpreter configuration");
- ParagraphTextParser.parse("%spark.pyspark(pool=");
}
@Test
public void testParagraphTextUnfinishedQuote() {
- exceptionRule.expect(RuntimeException.class);
- exceptionRule.expectMessage(
+ assertThrows(RuntimeException.class, () ->
+ ParagraphTextParser.parse("%spark.pyspark(pool=\"2314234) sc.version"),
"Problems by parsing paragraph. Not finished interpreter configuration");
- ParagraphTextParser.parse("%spark.pyspark(pool=\"2314234) sc.version");
}
@Test
public void testParagraphTextUnclosedBackslash() {
- exceptionRule.expect(RuntimeException.class);
- exceptionRule.expectMessage(
+ assertThrows(RuntimeException.class, () ->
+ ParagraphTextParser.parse("%spark.pyspark(pool=\\"),
"Problems by parsing paragraph. Unfinished escape sequence");
- ParagraphTextParser.parse("%spark.pyspark(pool=\\");
}
@Test
public void testParagraphTextEmptyKey() {
- exceptionRule.expect(RuntimeException.class);
- exceptionRule.expectMessage(
- "Problems by parsing paragraph. Local property key is empty");
- ParagraphTextParser.parse("%spark.pyspark(pool=123, ,)");
+ assertThrows((RuntimeException.class), () ->
+ ParagraphTextParser.parse("%spark.pyspark(pool=123, ,)"),
+ "Problems by parsing paragraph. Local property key is empty");
Review Comment:
The same with the above.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4532:
URL: https://github.com/apache/zeppelin/pull/4532#discussion_r1044995443
##########
zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/ParagraphTextParserTest.java:
##########
@@ -136,38 +136,32 @@ public void testParagraphTextQuotedPropertyKeyAndValue() {
assertEquals("", parseResult.getScriptText());
}
- @Rule
- public ExpectedException exceptionRule = ExpectedException.none();
@Test
public void testParagraphTextUnfinishedConfig() {
- exceptionRule.expect(RuntimeException.class);
- exceptionRule.expectMessage(
+ assertThrows(RuntimeException.class, () ->
+ ParagraphTextParser.parse("%spark.pyspark(pool="),
"Problems by parsing paragraph. Not finished interpreter configuration");
- ParagraphTextParser.parse("%spark.pyspark(pool=");
}
@Test
public void testParagraphTextUnfinishedQuote() {
- exceptionRule.expect(RuntimeException.class);
- exceptionRule.expectMessage(
+ assertThrows(RuntimeException.class, () ->
+ ParagraphTextParser.parse("%spark.pyspark(pool=\"2314234) sc.version"),
Review Comment:
It might have a different indentation scheme
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4532:
URL: https://github.com/apache/zeppelin/pull/4532#discussion_r1044995568
##########
zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTest.java:
##########
@@ -18,25 +18,25 @@
import org.apache.zeppelin.conf.ZeppelinConfiguration.ConfVars;
-import org.junit.After;
-import org.junit.Assert;
-import org.junit.BeforeClass;
-import org.junit.Test;
-
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
import java.net.MalformedURLException;
import java.util.List;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
Review Comment:
nit: Use a single line to divide sections
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on pull request #4532: [ZEPPELIN-5851][JUnit 5 Migration] Module: zeppelin-zengine
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on PR #4532:
URL: https://github.com/apache/zeppelin/pull/4532#issuecomment-1530969486
@huage1994 Please rebase your current branch. I merged #4582 right now.
--
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: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org