You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "olabusayoT (via GitHub)" <gi...@apache.org> on 2024/10/08 22:32:35 UTC

[PR] Add test as dependency to daffodil-test-integration [daffodil]

olabusayoT opened a new pull request, #1333:
URL: https://github.com/apache/daffodil/pull/1333

   - update failing udf test
   
   DAFFODIL-2933


-- 
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: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add test as dependency to daffodil-test-integration [daffodil]

Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT merged PR #1333:
URL: https://github.com/apache/daffodil/pull/1333


-- 
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: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add test as dependency to daffodil-test-integration [daffodil]

Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1333:
URL: https://github.com/apache/daffodil/pull/1333#discussion_r1797249316


##########
daffodil-test-integration/src/test/scala/org/apache/daffodil/cliTest/TestCLIUdfs.scala:
##########
@@ -64,8 +64,9 @@ class TestCLIUdfs {
     val schema = path(
       "daffodil-udf/src/test/resources/org/apache/daffodil/udf/genericUdfSchema.xsd"
     )
+    val classpath = udfClasspath()
 
-    runCLI(args"-v parse -s $schema -r user_func1") { cli =>
+    runCLI(args"-v parse -s $schema -r user_func1", classpath) { cli =>

Review Comment:
   I don't think the integration tests use the same classpath used for normal tests with SBT/IDE. These tests fork witha  completely custom classpath, so changes to the SBT config shouldn't affect, I 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: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add test as dependency to daffodil-test-integration [daffodil]

Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1333:
URL: https://github.com/apache/daffodil/pull/1333#discussion_r1797226673


##########
daffodil-test-integration/src/test/scala/org/apache/daffodil/cliTest/TestCLIUdfs.scala:
##########
@@ -64,8 +64,9 @@ class TestCLIUdfs {
     val schema = path(
       "daffodil-udf/src/test/resources/org/apache/daffodil/udf/genericUdfSchema.xsd"
     )
+    val classpath = udfClasspath()
 
-    runCLI(args"-v parse -s $schema -r user_func1") { cli =>
+    runCLI(args"-v parse -s $schema -r user_func1", classpath) { cli =>

Review Comment:
   So I think the addition of the daffodil-test as a dependency caused the internal classpath to have the UDFs available which caused the test to fail with the changes, so we had to explicitly set the the udfClasspath to be empty for it to pass.
   
   Before we didn't need to because the daffodil-test (which contains the UDF dependencies was missing)
   
   I can update with a comment for clarity though



-- 
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: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add test as dependency to daffodil-test-integration [daffodil]

Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1333:
URL: https://github.com/apache/daffodil/pull/1333#discussion_r1797339519


##########
daffodil-test-integration/src/test/scala/org/apache/daffodil/cliTest/TestCLIUdfs.scala:
##########
@@ -64,8 +64,9 @@ class TestCLIUdfs {
     val schema = path(
       "daffodil-udf/src/test/resources/org/apache/daffodil/udf/genericUdfSchema.xsd"
     )
+    val classpath = udfClasspath()
 
-    runCLI(args"-v parse -s $schema -r user_func1") { cli =>
+    runCLI(args"-v parse -s $schema -r user_func1", classpath) { cli =>

Review Comment:
   Ah, and that's the problem, this test doesn't fork so it will use the normal classpath. If you specify classpath then it will fork, which is way that fixed it. Probably all the tests in here want to either specify `fork = true` or ensure that something is defined that causes forking, like a non-empty classpath. I think in this test case we explicitly don't want a classpath to ensure no UDFs are loaded, so we should set fork = `true`



-- 
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: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add test as dependency to daffodil-test-integration [daffodil]

Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1333:
URL: https://github.com/apache/daffodil/pull/1333#discussion_r1797208911


##########
daffodil-test-integration/src/test/scala/org/apache/daffodil/cliTest/TestCLIUdfs.scala:
##########
@@ -64,8 +64,9 @@ class TestCLIUdfs {
     val schema = path(
       "daffodil-udf/src/test/resources/org/apache/daffodil/udf/genericUdfSchema.xsd"
     )
+    val classpath = udfClasspath()
 
-    runCLI(args"-v parse -s $schema -r user_func1") { cli =>
+    runCLI(args"-v parse -s $schema -r user_func1", classpath) { cli =>

Review Comment:
   Was it intentional to not add the udfClasspath for this tests since this test is about not having any UDF's? Probably worth adding a comment to make that clear.
   
   Though, I would have expected the CLI to succeed with UDF's on the classpath so maybe I'm missing something?



-- 
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: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org