You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "dongxuwang (via GitHub)" <gi...@apache.org> on 2023/08/16 03:25:32 UTC
[GitHub] [incubator-pekko] dongxuwang opened a new pull request, #575: add name with space test case for ActorSystemSpec
dongxuwang opened a new pull request, #575:
URL: https://github.com/apache/incubator-pekko/pull/575
Add a more common bad case for system name
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #575: add name with space test case for ActorSystemSpec
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #575:
URL: https://github.com/apache/incubator-pekko/pull/575#discussion_r1295392357
##########
actor-tests/src/test/scala/org/apache/pekko/actor/ActorSystemSpec.scala:
##########
@@ -135,6 +135,7 @@ class ActorSystemSpec extends PekkoSpec(ActorSystemSpec.config) with ImplicitSen
for (n <- Seq(
"-hallowelt",
"_hallowelt",
+ "hallo welt",
"hallo*welt",
"hallo@welt",
"hallo#welt",
Review Comment:
Yes, I think that should be better, what do you think.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] dongxuwang commented on a diff in pull request #575: update ActorSystemSpec to use property based testing for its name
Posted by "dongxuwang (via GitHub)" <gi...@apache.org>.
dongxuwang commented on code in PR #575:
URL: https://github.com/apache/incubator-pekko/pull/575#discussion_r1295830545
##########
actor-tests/src/test/scala/org/apache/pekko/actor/ActorSystemSpec.scala:
##########
@@ -15,15 +15,13 @@ package org.apache.pekko.actor
import java.util.concurrent.{ ConcurrentLinkedQueue, RejectedExecutionException }
import java.util.concurrent.atomic.AtomicInteger
-
import scala.concurrent.{ Await, Future }
import scala.concurrent.duration._
import scala.language.postfixOps
-
import scala.annotation.nowarn
import com.typesafe.config.{ Config, ConfigFactory }
-
import org.apache.pekko
+import org.scalatestplus.scalacheck.ScalaCheckPropertyChecks
Review Comment:
updated
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] dongxuwang commented on a diff in pull request #575: update ActorSystemSpec to use property based testing for its name
Posted by "dongxuwang (via GitHub)" <gi...@apache.org>.
dongxuwang commented on code in PR #575:
URL: https://github.com/apache/incubator-pekko/pull/575#discussion_r1295562508
##########
actor-tests/src/test/scala/org/apache/pekko/actor/ActorSystemSpec.scala:
##########
@@ -125,23 +123,17 @@ object ActorSystemSpec {
}
@nowarn
-class ActorSystemSpec extends PekkoSpec(ActorSystemSpec.config) with ImplicitSender {
+class ActorSystemSpec extends PekkoSpec(ActorSystemSpec.config) with ImplicitSender with ScalaCheckPropertyChecks {
import ActorSystemSpec.FastActor
"An ActorSystem" must {
"reject invalid names" in {
- for (n <- Seq(
- "-hallowelt",
- "_hallowelt",
- "hallo*welt",
- "hallo@welt",
- "hallo#welt",
- "hallo$welt",
- "hallo%welt",
- "hallo/welt")) intercept[IllegalArgumentException] {
- ActorSystem(n)
+ forAll { (name: String) =>
+ whenever(!name.matches("""^[a-zA-Z0-9][a-zA-Z0-9-_]*$""")) {
+ intercept[IllegalArgumentException](ActorSystem(name))
+ }
Review Comment:
Makes sense. Actually I felt like adding one more common case "hallo welt".
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #575: update ActorSystemSpec to use property based testing for its name
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #575:
URL: https://github.com/apache/incubator-pekko/pull/575#discussion_r1295563652
##########
actor-tests/src/test/scala/org/apache/pekko/actor/ActorSystemSpec.scala:
##########
@@ -125,23 +123,17 @@ object ActorSystemSpec {
}
@nowarn
-class ActorSystemSpec extends PekkoSpec(ActorSystemSpec.config) with ImplicitSender {
+class ActorSystemSpec extends PekkoSpec(ActorSystemSpec.config) with ImplicitSender with ScalaCheckPropertyChecks {
import ActorSystemSpec.FastActor
"An ActorSystem" must {
"reject invalid names" in {
- for (n <- Seq(
- "-hallowelt",
- "_hallowelt",
- "hallo*welt",
- "hallo@welt",
- "hallo#welt",
- "hallo$welt",
- "hallo%welt",
- "hallo/welt")) intercept[IllegalArgumentException] {
- ActorSystem(n)
+ forAll { (name: String) =>
+ whenever(!name.matches("""^[a-zA-Z0-9][a-zA-Z0-9-_]*$""")) {
+ intercept[IllegalArgumentException](ActorSystem(name))
+ }
Review Comment:
keep the old test and add an extra new test that uses the property based approach
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #575: update ActorSystemSpec to use property based testing for its name
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #575:
URL: https://github.com/apache/incubator-pekko/pull/575#discussion_r1295526361
##########
actor-tests/src/test/scala/org/apache/pekko/actor/ActorSystemSpec.scala:
##########
@@ -125,23 +123,17 @@ object ActorSystemSpec {
}
@nowarn
-class ActorSystemSpec extends PekkoSpec(ActorSystemSpec.config) with ImplicitSender {
+class ActorSystemSpec extends PekkoSpec(ActorSystemSpec.config) with ImplicitSender with ScalaCheckPropertyChecks {
import ActorSystemSpec.FastActor
"An ActorSystem" must {
"reject invalid names" in {
- for (n <- Seq(
- "-hallowelt",
- "_hallowelt",
- "hallo*welt",
- "hallo@welt",
- "hallo#welt",
- "hallo$welt",
- "hallo%welt",
- "hallo/welt")) intercept[IllegalArgumentException] {
- ActorSystem(n)
+ forAll { (name: String) =>
+ whenever(!name.matches("""^[a-zA-Z0-9][a-zA-Z0-9-_]*$""")) {
+ intercept[IllegalArgumentException](ActorSystem(name))
+ }
Review Comment:
I prefer the old way - you can actually see what is being tested. With the change, I have to hope that use cases are tested.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] dongxuwang commented on a diff in pull request #575: add name with space test case for ActorSystemSpec
Posted by "dongxuwang (via GitHub)" <gi...@apache.org>.
dongxuwang commented on code in PR #575:
URL: https://github.com/apache/incubator-pekko/pull/575#discussion_r1295407567
##########
actor-tests/src/test/scala/org/apache/pekko/actor/ActorSystemSpec.scala:
##########
@@ -135,6 +135,7 @@ class ActorSystemSpec extends PekkoSpec(ActorSystemSpec.config) with ImplicitSen
for (n <- Seq(
"-hallowelt",
"_hallowelt",
+ "hallo welt",
"hallo*welt",
"hallo@welt",
"hallo#welt",
Review Comment:
Yes, agree
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #575: add name with space test case for ActorSystemSpec
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #575:
URL: https://github.com/apache/incubator-pekko/pull/575#discussion_r1295395107
##########
actor-tests/src/test/scala/org/apache/pekko/actor/ActorSystemSpec.scala:
##########
@@ -135,6 +135,7 @@ class ActorSystemSpec extends PekkoSpec(ActorSystemSpec.config) with ImplicitSen
for (n <- Seq(
"-hallowelt",
"_hallowelt",
+ "hallo welt",
"hallo*welt",
"hallo@welt",
"hallo#welt",
Review Comment:
```scala
if (!name.matches("""^[a-zA-Z0-9][a-zA-Z0-9-_]*$"""))
throw new IllegalArgumentException(
"invalid ActorSystem name [" + name +
"], must contain only word characters (i.e. [a-zA-Z0-9] plus non-leading '-' or '_')")
```
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #575: update ActorSystemSpec to use property based testing for its name
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #575:
URL: https://github.com/apache/incubator-pekko/pull/575#discussion_r1295559748
##########
actor-tests/src/test/scala/org/apache/pekko/actor/ActorSystemSpec.scala:
##########
@@ -125,23 +123,17 @@ object ActorSystemSpec {
}
@nowarn
-class ActorSystemSpec extends PekkoSpec(ActorSystemSpec.config) with ImplicitSender {
+class ActorSystemSpec extends PekkoSpec(ActorSystemSpec.config) with ImplicitSender with ScalaCheckPropertyChecks {
import ActorSystemSpec.FastActor
"An ActorSystem" must {
"reject invalid names" in {
- for (n <- Seq(
- "-hallowelt",
- "_hallowelt",
- "hallo*welt",
- "hallo@welt",
- "hallo#welt",
- "hallo$welt",
- "hallo%welt",
- "hallo/welt")) intercept[IllegalArgumentException] {
- ActorSystem(n)
+ forAll { (name: String) =>
+ whenever(!name.matches("""^[a-zA-Z0-9][a-zA-Z0-9-_]*$""")) {
+ intercept[IllegalArgumentException](ActorSystem(name))
+ }
Review Comment:
Then there will be infinite cases to add
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #575: add name with space test case for ActorSystemSpec
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #575:
URL: https://github.com/apache/incubator-pekko/pull/575#discussion_r1295343065
##########
actor-tests/src/test/scala/org/apache/pekko/actor/ActorSystemSpec.scala:
##########
@@ -135,6 +135,7 @@ class ActorSystemSpec extends PekkoSpec(ActorSystemSpec.config) with ImplicitSen
for (n <- Seq(
"-hallowelt",
"_hallowelt",
+ "hallo welt",
"hallo*welt",
"hallo@welt",
"hallo#welt",
Review Comment:
This maybe better with some random generation properties tests.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] dongxuwang commented on a diff in pull request #575: update ActorSystemSpec to use property based testing for its name
Posted by "dongxuwang (via GitHub)" <gi...@apache.org>.
dongxuwang commented on code in PR #575:
URL: https://github.com/apache/incubator-pekko/pull/575#discussion_r1295591392
##########
actor-tests/src/test/scala/org/apache/pekko/actor/ActorSystemSpec.scala:
##########
@@ -125,23 +123,17 @@ object ActorSystemSpec {
}
@nowarn
-class ActorSystemSpec extends PekkoSpec(ActorSystemSpec.config) with ImplicitSender {
+class ActorSystemSpec extends PekkoSpec(ActorSystemSpec.config) with ImplicitSender with ScalaCheckPropertyChecks {
import ActorSystemSpec.FastActor
"An ActorSystem" must {
"reject invalid names" in {
- for (n <- Seq(
- "-hallowelt",
- "_hallowelt",
- "hallo*welt",
- "hallo@welt",
- "hallo#welt",
- "hallo$welt",
- "hallo%welt",
- "hallo/welt")) intercept[IllegalArgumentException] {
- ActorSystem(n)
+ forAll { (name: String) =>
+ whenever(!name.matches("""^[a-zA-Z0-9][a-zA-Z0-9-_]*$""")) {
+ intercept[IllegalArgumentException](ActorSystem(name))
+ }
Review Comment:
updated, PTAL
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #575: update ActorSystemSpec to use property based testing for its name
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #575:
URL: https://github.com/apache/incubator-pekko/pull/575#discussion_r1295619576
##########
actor-tests/src/test/scala/org/apache/pekko/actor/ActorSystemSpec.scala:
##########
@@ -15,15 +15,13 @@ package org.apache.pekko.actor
import java.util.concurrent.{ ConcurrentLinkedQueue, RejectedExecutionException }
import java.util.concurrent.atomic.AtomicInteger
-
import scala.concurrent.{ Await, Future }
import scala.concurrent.duration._
import scala.language.postfixOps
-
import scala.annotation.nowarn
import com.typesafe.config.{ Config, ConfigFactory }
-
import org.apache.pekko
+import org.scalatestplus.scalacheck.ScalaCheckPropertyChecks
Review Comment:
can you move this import away from the pekko ones?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning merged pull request #575: update ActorSystemSpec to use property based testing for its name
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning merged PR #575:
URL: https://github.com/apache/incubator-pekko/pull/575
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] dongxuwang commented on a diff in pull request #575: add name with space test case for ActorSystemSpec
Posted by "dongxuwang (via GitHub)" <gi...@apache.org>.
dongxuwang commented on code in PR #575:
URL: https://github.com/apache/incubator-pekko/pull/575#discussion_r1295365796
##########
actor-tests/src/test/scala/org/apache/pekko/actor/ActorSystemSpec.scala:
##########
@@ -135,6 +135,7 @@ class ActorSystemSpec extends PekkoSpec(ActorSystemSpec.config) with ImplicitSen
for (n <- Seq(
"-hallowelt",
"_hallowelt",
+ "hallo welt",
"hallo*welt",
"hallo@welt",
"hallo#welt",
Review Comment:
You mean to replace all those strings with a random generation?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org