-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
dotty.tools.scripting.BashScriptsTests are not running properly, but tests are succeeding #12962
Comments
This is in CI (as opposed to just when tests are run locally), so I think it merits a priority bump. |
@philwalk do you think this is an easy fix? |
I'll look at today, it sounds like an easy fix. |
Looking at the results of the CI run, the path seems to be incorrect
|
I'm not yet familiar with the CI run, is The directory |
It will not be generated unless
|
So should |
Other tests that require I'm not sure if these tests are suitable (or can be adapted) to be run from there also. |
The purpose of I think the easiest fix here is to have |
If the existence of I think that guaranteeing the order in which BashScriptsTests are executed after dist/pack will help stabilize the tests. |
Moreover I would love to see a change such that the failure is fatal to the test/CI. 🙏🏼 |
It seems possible that we could just modify .github/workflows/ci.yaml, by replacing this line:
with this:
I'm not sure how to run the CI build on my system in order to test this approach. but I'm happy to learn, given some documentation. |
That would ensure that the test passes. What I meant is the fact that with the previous PR the test was still silently failing. |
I think the test should fail if the scala command does not exist on the path. |
Ok, I think I understand now.
That makes sense. I should be able to push something soon. Here's the proposed fix: #13029 |
That line is part of the |
With the new PR, there is a failure for one of the tests: |
BashScriptsTests should be changed to run on |
Just an FYI, the only thing this test does is to run a script that prints all command line arguments to STDOUT, for the purpose of verifying that |
Executable scala commands are not obtained until If Rather than the test environment, I think the issue is whether or not to run the test in a bootstrapped state. |
@magnolia-k |
How to detect whether the test is running in a bootstrapped state? Is it indicated by the value of property "user.dir", or is there another way? |
IMO the 3 test files
Here is what I see from the output of the CI job
And here what I get in my local build on a Win10 laptop after fixing the file paths (both executables and file arguments) :
In short my local solution looks as follows :
|
Can you clarify, or give an example? BTW, your proposal sounds good. |
@michelou - The current assumption in We could alternatively indicate where they are via environment variables or System properties. |
@philwalk Here is an example of the
NB. Java class file versions : 61 ➞ Java 17, 55 ➞ Java 11, 52 ➞ Java 8. And here is the version of Java found in
NB. On MS Windows 10 Java 8 is installed by default in the WSL Ubuntu distribution. In my case I upgraded my installation to Java 11 to get rid of the issue between Java 8 and Java 11 (but now I have the same issue between Java 11 and Java 17) when running the scripting tests. |
@michelou -
Is that correct? /mnt/c/Program Files/Java/jdk-11.0.12 Correct? FWIW, the following approach might help, although I'm also thinking there needs to be an easy way for tests to query the test environment for the correct settings of JAVA_HOME, SCALA_HOME,
|
@michelou - You can now specify the working directory, TEST_BASH=/mnt/c/Windows/System32/bash.exe
TEST_CWD=/mnt/c/dotty
JAVA_HOME='/mnt/c/Program Files/Java/jdk-11.0.12'
SCALA_HOME='/mnt/c/dotty/dist/target/pack'
PATH="$JAVA_HOME/bin:$SCALA_HOME/bin:$PATH" If I haven't yet refactored |
Right, my function
Right. All my Java implementations are actually installed in directory
|
@michelou - are you able to achieve what you need with the most recent changes to |
…s-12962 fail verifyScalacArgs if dist/target/pack/bin/scalac not found - fix for #12962
I think this issue is resolved. Are there any residual concerns? |
Resolved until proved guilty. |
[info] Test run started [info] Test dotty.tools.scripting.BashScriptsTests.verifyScalaArgs started osname[linux] bashExe: [/bin/bash] /bin/bash: /home/vagrant/dotty/compiler/target/scala-3.0.0/test-classes/scripting/showArgs.sc: Permission denied [info] Test dotty.tools.scripting.BashScriptsTests.verifyScalacArgs started osname[linux] bashExe: [/bin/bash] /bin/bash: -script: command not found [info] Test run finished: 0 failed, 0 ignored, 2 total, 0.061s
BashScriptsTests
uses thewhich
function provided byClasspathTests
to get the path of scala and scalac commands, but this function depends on the environment variable PATH, so it cannot get the path of the built binary.As a result, the
which
function returns an empty string, andBashScriptsTests
interprets the following first argument as a command and tries to execute it.The text was updated successfully, but these errors were encountered: