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

Migrate scripted tests for sbt bridge from zinc repo [ci: last-only] #10554

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented Sep 22, 2023

Build changes copied and adapted from lampepfl/dotty. Tests copied and adapted from sbt/zinc.

To Do

  • source-dependencies/patMat-scope: after first change, compilation only seems to compile changed file. Existing ticket: IncOptions.useOptimizedSealed is broken on 2.13 sbt/zinc#1229
  • source-dependencies/empty-package: after setting withRecompileAllFraction to 1.0 this started failing. Old issue, logged at Missing invalidation when deleting source file sbt/zinc#1268
  • Implement missing check operations: checkMainClasses, checkIterations, checkWarnings, checkRecompilations, checkDependencies, checkClasses, checkProducts, checkNoClassFiles, checkProductsExists, checkNameExistsInClass, re-enable tests that were disabled in the corresponding commit (one of this PR)
  • check defaults that are different in zinc's scripted implementation (https://github.com/sbt/zinc/blob/9431f67d02feac2456aea50d194e903008f7de79/internal/zinc-scripted/src/test/scala/sbt/internal/inc/IncHandler.scala#L294)
  • figure out when to run the scripted tests in CI done
    • On my machine running them all takes about 15 minutes
    • In the zinc repo tests run much faster; zinc is not using sbt's implementation for scripted tests but rather has its own re-implementation of the a "scripted" test framework.
    • Setting scriptedBatchExecution := true would make them run much faster, however there's cross-talk between tests that makes some of them fail (e.g., recompilation counts don't start at 1). In dotty some such issues were adressed in https://github.com/lampepfl/dotty/pull/10498/commits, but I'd prefer to keep that complexity out.
    • The fact that each test performs a reload (> setup; reload to include the shared settings) doesn't seem to affect performance too much (tested by copying the shared sbt file to each test directory).
  • Check if there are tests that seem irrelevant to testing the compiler bridge and could be deleted.

@lrytz lrytz requested a review from dwijnand September 22, 2023 13:28
@scala-jenkins scala-jenkins added this to the 2.13.13 milestone Sep 22, 2023
build.sbt Outdated Show resolved Hide resolved
@SethTisue SethTisue added the internal not resulting in user-visible changes (build changes, tests, internal cleanups) label Sep 25, 2023
@lrytz lrytz changed the title Migrate scripted tests for sbt bridge from zinc repo Migrate scripted tests for sbt bridge from zinc repo [ci: last-only] Oct 3, 2023
@lrytz lrytz force-pushed the bridge-tests branch 2 times, most recently from c70cd1b to ebc72cf Compare October 3, 2023 08:45
@lrytz lrytz marked this pull request as ready for review October 3, 2023 08:45
@lrytz lrytz force-pushed the bridge-tests branch 4 times, most recently from be90719 to 1625c60 Compare October 4, 2023 14:54
@lrytz
Copy link
Member Author

lrytz commented Oct 13, 2023

This is done now, modulo deciding how to run the tests.

@SethTisue see my comments in the PR description. What do you think?

@lrytz
Copy link
Member Author

lrytz commented Nov 10, 2023

Managed to get scriptedBatchExecution := true working, which also opened up the door for scriptedParallelInstances := 2. Now the whole suite runs in 2:11 on my computer 🏁 so we can enable it in CI (already done in this PR). Thanks @dwijnand for the help!

Copy link
Member

@SethTisue SethTisue left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe squash into fewer commits before merging?

@lrytz
Copy link
Member Author

lrytz commented Nov 14, 2023

👍 will do

Copied and adapted from lampepfl/dotty
- use scriptedBatchExecution and scriptedParallelInstances
- set scala version in tests
- implement shared settings
- delete unrelated tests
- adjust tests for fullName until PR 10542 is merged
- adjust test to hard-coded max filename length of 240, it was
  255 on 2.12 (in the zinc repo)
@lrytz lrytz merged commit 96128c8 into scala:2.13.x Nov 14, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal not resulting in user-visible changes (build changes, tests, internal cleanups)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants