[GitHub] stevedlawrence commented on a change in pull request #170: Fixed CLI integration tests so that they run properly on Windows.

2019-01-28 Thread GitBox
stevedlawrence commented on a change in pull request #170: Fixed CLI 
integration tests so that they run properly on Windows.
URL: https://github.com/apache/incubator-daffodil/pull/170#discussion_r251569006
 
 

 ##
 File path: build.sbt
 ##
 @@ -139,6 +139,7 @@ lazy val commonSettings = Seq(
   sourceManaged := baseDirectory.value / "src_managed",
   resourceManaged := baseDirectory.value / "resource_managed",
   libraryDependencies ++= Dependencies.common,
+  parallelExecution in IntegrationTest := false
 
 Review comment:
   Seems reasonble in that case. The change only disables parallel executiong 
for integration tests (i.e. CLI tests), so it shouldn't affect the time it 
takes to run all our normal tests. As Josh mentions, we could disable/enable 
parallel execution based on the operating system, but I think I'd rather 
evertyhing be as similar as possible on different OS. So if it needs to be 
disabled on Windows, I'd vote for also disabling on Linux too.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] stevedlawrence commented on a change in pull request #170: Fixed CLI integration tests so that they run properly on Windows.

2019-01-28 Thread GitBox
stevedlawrence commented on a change in pull request #170: Fixed CLI 
integration tests so that they run properly on Windows.
URL: https://github.com/apache/incubator-daffodil/pull/170#discussion_r251411702
 
 

 ##
 File path: build.sbt
 ##
 @@ -139,6 +139,7 @@ lazy val commonSettings = Seq(
   sourceManaged := baseDirectory.value / "src_managed",
   resourceManaged := baseDirectory.value / "resource_managed",
   libraryDependencies ++= Dependencies.common,
+  parallelExecution in IntegrationTest := false
 
 Review comment:
   Agreed, I'm not even sure this change is needed at all. I know Mark was 
running into memory issues when runing on windows and this was one change he 
made early on. But I'm not sure it really helped. I think the actual fix was 
changes to DAFFODIL_JAVA_OPTS in TestCLIDebugger.
   
   Mark, can you remove this change and see if the memory issues come back? If 
not, I think we should remove this change completely. 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] stevedlawrence commented on a change in pull request #170: Fixed CLI integration tests so that they run properly on Windows.

2019-01-28 Thread GitBox
stevedlawrence commented on a change in pull request #170: Fixed CLI 
integration tests so that they run properly on Windows.
URL: https://github.com/apache/incubator-daffodil/pull/170#discussion_r251393696
 
 

 ##
 File path: 
daffodil-cli/src/it/scala/org/apache/daffodil/tunables/TestCLITunables2.scala
 ##
 @@ -34,13 +34,13 @@ class TestCLITunables2 {
 
 try {
   // note: 2>&1 is shell-speak for "connect stderr into stdout"
-  val cmd = String.format("""echo "a,b" | %s parse -s %s 
-TsuppressSchemaDefinitionWarnings=all 2>&1""", Util.binPath, testSchemaFile)
+  val cmd = String.format("""echo a,b| %s parse -s %s 
-TsuppressSchemaDefinitionWarnings=all 2>&1""", Util.binPath, testSchemaFile)
 
 Review comment:
   The new ``Util.echoN`` function is actually only needed when the ``-n`` flag 
is used. The echo command behaves exactly the same on linux in windows if the 
following conditions are met:
   
   1. No flags are provided (i.e. ``-n``)
   1. No quotes are needed
   1. No spaces exists at the end of the echo string before the pipe
   
   If these are all true, then the only difference between windows and linux 
echo is CRLF vs LF, which the schemes in most of these tests handle with 
``%NL;``. In this particular test, we needed to remove the quotes and the space 
before the pipe to make it work on linux and windows, but the schema is fine 
with the newline differences. In cases where the ``-n`` flag was used for 
linux, the new ``Util.echoN`` function was created, which creates the right 
command to echo without a newline at the end based on the operating system. So 
only tests that do not expect a ``%NL;`` need use use the new function.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services