Skip to content

fix ExpressionTest#verifyImports test compile error introduced in #14263 #14358

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

Merged
merged 3 commits into from
Jan 28, 2022
Merged

fix ExpressionTest#verifyImports test compile error introduced in #14263 #14358

merged 3 commits into from
Jan 28, 2022

Conversation

philwalk
Copy link
Contributor

The -e (eval) expression in dotty.tools.scripting.ExpressionTest#verifyImports fails to compile, but the test passes anyway because of a missing assertion.

This PR corrects the expression so that it compiles, and it asserts that the expression must print an integer to STDOUT, so that the test will fail if the -e <expression> implementation fails.

The compile error is mentioned in #14332

@philwalk philwalk requested a review from michelou January 26, 2022 17:52
Copy link
Contributor

@michelou michelou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First 2 remarks about the code in ExpressionTest.scala :

  1. On line 30, the 's' prefix is not need in front of the triple quoted string.
  2. On line 39, the variable cmd is unused.

Then 2 questions about the usage of bashCommand in ExpressionTest.scala :

  1. In file BashScriptTests.scala bashCommand makes use of ScriptTestEnv.scalaPath; why not use the same mechanism here ?
    E.g., val (_, _, stdout, stderr) = bashCommand(s"""$scalaPath -e '$expression'""").
  2. In variable expressionLines (line 28) I read Paths.get("\""."\""); for me the code only works if I write Paths.get("\"."\") (see console output below). Did you check the console output starting with bashCmd ?

Finally, adding assert(success) (line 36) is good !

PS. Here is the output I get with my local copy of file ExpressionTest.scala (Win11 Pro, Scala 3.1.0):

Y:\dotty>"%SCALA3_HOME%\bin\scalac" -cp "%REPO_DIR%\junit\junit\4.13.2\junit-4.13.2.jar;compiler\target\scala-3.1.2-RC1\test-classes" compiler\test\dotty\tools\scripting\ExpressionTest.scala -d c:\temp\

Y:\dotty>"%SCALA3_HOME%\bin\scala" -cp "%REPO_DIR%\junit\junit\4.13.2\junit-4.13.2.jar;c:\temp;compiler\target\scala-3.1.2-RC1\classes;compiler\target\scala-3.1.2-RC1\test-classes" dotty.tools.scripting.ExpressionTest
working directory is [Y:/dotty]

=== verifyCommandLineExpression ===
===> verify -e <expression> is properly handled by `dist/bin/scala`
bashCmd: C:/windows/system32/bash.exe -c "/mnt/c/Users/michelou/workspace-perso/dotty-examples/dotty/dist/target/pack/bin/scala -e 'println(3*3)'"
scalacPath: Y:/dotty/dist/target/pack/bin/scalac
JAVA_HOME : C:\opt\jdk-temurin-1.8.0u322-b06
SCALA_HOME : Y:/dotty/dist/target/pack
PATH : dist/target/pack/bin;C:/opt/jdk-temurin-1.8.0u322-b06/bin;[...]
SHELLOPTS : igncr:braceexpand:hashall:ignoreeof:monitor:vi
stdout: 9
stderr:

=== verifyImports ===
bashCmd: C:/windows/system32/bash.exe -c "/mnt/c/Users/michelou/workspace-perso/dotty-examples/dotty/dist/target/pack/bin/scala -e 'import java.nio.file.Paths;println(Paths.get("\"."\").toFile.listFiles.toList.filter(_.isDirectory).size)'"
stdout: 19
stderr:

Note: I appended the following code to file ExpressionTest.scala to get the above output :

object ExpressionTest:

  def main(args: Array[String]): Unit =
    val tests = new ExpressionTest
    println("\n=== verifyCommandLineExpression ===")
    tests.verifyCommandLineExpression
    println("\n=== verifyImports ===")
    tests.verifyImports

@michelou
Copy link
Contributor

@philwalk 3 more suggestions for file ExpressionTest.scala :

  1. line 28 : remove empty line in function verifyImports
  2. line 31 : use scala.util.Properties.userDir instead of dotty.tools.dotc.config.Properties.userDir.
  3. line 46 : remove opening and closing braces (indentation is enough)

Otherwise, LGTM.

@philwalk
Copy link
Contributor Author

@michelou - thanks for the detailed feedback, that helps!

It turns out that changing the bash command to s"""$scalaPath -e '$expression'""" breaks the test on my system.
I spent quite some time trying to figure out exactly what was happening, but in any event, it's related to the expression having double quotes, which are consumed when bash interpolates. To avoid the problem, I removed the double quotes altogether, rather than trying to escape them.

I just noticed the code still has a residual unused 's' prefix (this time it's on line 32!).

@philwalk
Copy link
Contributor Author

I'll do another push with all suggested changes after the workflow dies down a bit later this evening.

@michelou
Copy link
Contributor

I'll do another push with all suggested changes after the workflow dies down a bit later this evening.

👍

@philwalk
Copy link
Contributor Author

@michelou -
I'm inclined to add your suggested main method at the bottom:

object ExpressionTest:

  def main(args: Array[String]): Unit =
    val tests = new ExpressionTest
    println("\n=== verifyCommandLineExpression ===")
    tests.verifyCommandLineExpression
    println("\n=== verifyImports ===")
    tests.verifyImports

@michelou
Copy link
Contributor

@michelou - I'm inclined to add your suggested main method at the bottom:

object ExpressionTest:
  //...

It's a small (local) addition I also have in BashScriptTests; it helps me to quickly experiment with the bash.exe arguments on Windows (as illustrated in the above postscriptum).

@anatoliykmetyuk anatoliykmetyuk merged commit 746b9c6 into scala:master Jan 28, 2022
@philwalk philwalk deleted the expressionTest-bugfix branch January 28, 2022 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants