Skip to content
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

Refactor common scripting test code #14108

Closed
wants to merge 2 commits into from
Closed

Refactor common scripting test code #14108

wants to merge 2 commits into from

Conversation

philwalk
Copy link
Contributor

@philwalk philwalk commented Dec 14, 2021

This addresses issues described in #12962
It also is relevant to #14073

Refactor test sources:

compiler/test/dotty/tools/scripting/BashScriptsTests.scala
compiler/test/dotty/tools/scripting/ClasspathTests.scala
compiler/test/dotty/tools/scripting/ScriptingTests.scala

Common code is moved to a new file:

compiler/test/dotty/tools/scripting/ScriptTestEnv.scala

So called invalid tests (where permissions fail when running bash command lines) are now treated by default as test failures, although this behavior can be overridden, if needed, by defining PASS_INVALID_TESTS.

Copy link
Contributor

@BarkingBad BarkingBad left a comment

Choose a reason for hiding this comment

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

Looks good. Sadly, you didn't add [test_non_bootstrapped] and [test_windows_full] in body of your PR to enable these checks.

I think we need to add this trick to CI
types: [ opened, synchronize, reopened, edited ]
from here https://frontside.com/blog/2020-05-26-github-actions-pull_request/#how-do-workflows-trigger-on-pull_request so we could enable the CI without reopening the PR

@philwalk
Copy link
Contributor Author

philwalk commented Dec 14, 2021

add [test_non_bootstrapped] and [test_windows_full] in body of your PR to enable these checks.

So if I add those strings to the PR text, it will enable the addition tests? Ah, I now see the link you provided, I'll read it.

@BarkingBad
Copy link
Contributor

Unfortunately, editing text body now will have no effect, because we don't have edited action listed in the ci.yaml :( We have to open a separate PR with these 2 tags

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.

@philwalk Here are my suggestions :

File scripting/BashScriptsTests.scala

  • who does remove the temporary file create on line 21 ?!
    Tip : use annotation org.junit.AfterClass.
  • improve comment text on line 59;
    e.g. verify dist/bin/scala with -J setting and Scala script file.
    NB. The same remark applies to the next 3 test functions (-J and -D).
  • variable mismatchesin unused on line 98.

File scripting/ScriptTestEnv.scala

  • remove 1st (empty) line of file.
  • add comment text on line 15; in particular explain the usage of variables TEST_CWD (line 24) and TEST_BASH (line 31).
  • add a comment on line 68 to explain the values returned by the bashCommand function !
  • add a comment to explain why we may encounter "permission denied" on line 79.
  • indent case's in match statement on line 141.

@BarkingBad BarkingBad linked an issue Dec 14, 2021 that may be closed by this pull request
@philwalk philwalk mentioned this pull request Dec 14, 2021
@philwalk
Copy link
Contributor Author

philwalk commented Dec 14, 2021

@michelou - thanks for the feedback, all your suggestions are incorporated in #14113.
We added those and other fixes to a new PR, in order to be able to automatically trigger additional tests:

[test_windows_full]
[test_non_bootstrapped]

@BarkingBad
Copy link
Contributor

No longer valid

@BarkingBad BarkingBad closed this Dec 16, 2021
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.

Nightly Dotty workflow of 2021-12-14 failed
3 participants