Skip to content

Segregate scala2-interop-tests and add to CI #9990

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

Closed
wants to merge 4 commits into from

Conversation

griggt
Copy link
Contributor

@griggt griggt commented Oct 12, 2020

This PR moves the Scala 2 interop tests currently located in the sbt-dotty project to a standalone scala2-interop-tests project, and includes these tests in the test_bootstrapped job in CI.

Separating these tests allows them to be run independently from the sbt-dotty test suite, which is slow and only runs during the nightly scheduled CI. It also gives them a home for when/if sbt-dotty ceases to exist.

Adding the tests to the test_bootstrapped job means any regressions are noticed sooner, and CI configuration overrides should not be needed when fixing Scala 2 interop issues such as #9916, #6484, and #3100.

Copy link
Member

@dottybot dottybot left a comment

Choose a reason for hiding this comment

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

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

@smarter
Copy link
Member

smarter commented Oct 12, 2020

This PR moves the Scala 2 interop tests currently located in the sbt-dotty project to a standalone scala2-interop-tests project

I feel like we already have too many projects in our build which is getting overwhelming (and we're not even done adding more: #9925). If we were to move scripted tests somewhere, I would rather move them to the dotty-compiler project.

Separating these tests allows them to be run independently from the sbt-dotty test suite

That can be done by running sbt-dotty/scripted scala2-compat/*

It also gives them a home for when/if sbt-dotty ceases to exist.

Even if we remove sbt-dotty, we should still keep all the scripted tests in the dotty repo since they're integration tests for dotty.

Adding the tests to the test_bootstrapped job means any regressions are noticed sooner

That seems useful indeed, but could be done by adding sbt-dotty/scripted scala2-compat/* to the set of things run by test_bootstrapped (test_sbt would then also be running those tests but that's not a big deal I think, and could be avoided by also tweaking test_sbt to explicitly list what scripted tests we want it to run).

To recap, if we were to move the scripted tests I would rather move all of them (because we want to keep them all, and because having them spread across multiple projects would be confusing I think) and move them to an existing project. But doing that is orthogonal to how we want to split things up in our CI.

@griggt
Copy link
Contributor Author

griggt commented Oct 12, 2020

Fair points.

I still feel that it would be valuable to run the tests during test_bootstrapped.

Also, having dotty compiler integration tests in the sbt-dotty project just feels weird to me, but I certainly understand your reluctance to add even more projects to the build.

How would you suggest I proceed with this PR?

@smarter
Copy link
Member

smarter commented Oct 12, 2020

I propose not moving anything for now, and instead tweaking the CI config to run the scala2 tests in test_bootstrapped. Also, to avoid any unnecessary slowdown of test_bootstrapped, I think the eff and akka tests should be combined in just one project, since each sbt scripted test requires restarting sbt that would shave off a bit of time.

@griggt
Copy link
Contributor Author

griggt commented Oct 12, 2020

The good news is that even with five separate sbt scripted test projects (akka, eff, plus three new ones for 9916), it only added 107 seconds to the run time of test_bootstrapped. I'll combine them to avoid the sbt restarts and see what sort of time savings we achieve.

@griggt
Copy link
Contributor Author

griggt commented Oct 13, 2020

Some informal testing on my local workstation indicates that the per-test sbt startup cost is around 10-12 seconds. The CI runners may be slightly slower.

This overhead does dominate the total runtime of the scripted tests, and the standard project-per-test strategy scales poorly. However, merging the tests comes with a cost of loss of information. A unified test project means that testing halts at the first failure. Also, if there are multiple test items in pending, you are not guaranteed to get an accurate view of which tests still fail and which now succeed.

Given that the total number of scripted tests is currently low (2 + whatever is introduced for #9916), and the total runtime for test_bootstrapped is already 20-60 minutes, I am personally inclined the keep the individual scripted tests for now to avoid the limitations above. If it's deemed that minimization of runtime cost is of paramount importance, I can certainly merge the tests (and have already done so in my local branch).

I would prefer a solution that avoids sbt entirely (especially for tests that have no external dependencies), but thought it prudent to start with a smaller, simpler improvement to get the Scala 2 interop tests run more frequently.

@sjrd
Copy link
Member

sjrd commented Oct 13, 2020

In an ideal world, this wouldn't use sbt at all, indeed. It should be orchestrated by Vulpix (even with Vulpix forking for scalac compilation, it would still be a huge improvement to the status quo).

@smarter
Copy link
Member

smarter commented Oct 14, 2020

Closed in favor of #10000

@smarter smarter closed this Oct 14, 2020
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.

4 participants