Skip to content

convert partests that only need the reference compiler to be run by JUnit instead #295

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
SethTisue opened this issue Feb 2, 2017 · 10 comments

Comments

@SethTisue
Copy link
Member

currently our suite of partests is just a big bag of tests, some of which are only testing the standard library and don't need to be compiled with a freshly built compiler.

segregating library tests from compiler tests would be a win, all by itself, for organization, modularity, and maintainability.

but if we went on and changed the library tests to run without any involvement from partest at all, but by JUnit instead, there would be a number of further benefits:

  • the tests would run faster — the aggregate effect could make PR validation noticeably faster
  • the sources for these tests would become normal test sources in the sbt project and get various benefits from this such as all being compiled together in a warm compiler+JVM
  • the test names would become tab-completable in sbt and individually runnable in JUnit-aware IDEs
  • contributors would have less need to learn weird partest arcana (there is a lot to learn about how to run partests, how to write them, how to interpret failures, etc)

of course we already know about these benefits and that's why we prefer to write new tests in JUnit. but hand-translating existing partests to JUnit, one at a time, is too much work, so it doesn't happen that often.

we can minimize the changes needed to existing partests by writing a (hopefully fairly small) amount of new JUnit-based code to duplicate partest's .check file functionality. hopefully, then, moving a test out of partest will only require small hand changes such as:

  • adding a package declaration at the top
  • giving the test a unique name
  • perhaps adding the test to an (automatically updated?) registry so its existence wouldn't need to be discovered dynamically (apparently the Dotty folks have something like this?)

if it were that easy, getting large numbers of tests out of partest's clutches would actually become doable.

@vpetro wrote wip code for this today at a Lightbend engineering retreat, with help from @dwijnand and myself; we aim to produce at least a WIP PR on this soon.

@SethTisue
Copy link
Member Author

SethTisue commented Feb 2, 2017

@sjrd this could be of concern to Scala.js

@sjrd
Copy link
Member

sjrd commented Feb 2, 2017

As long as there is no I/O involved in the JUnit code used to "load" .check files or whatever, this wouldn't be a problem for Scala.js. We do run the JUnit tests of scala/scala as well as the partest tests. But of course if a JUnit test used java.io.File or something similar to load .check files, that would be an issue.

@SethTisue
Copy link
Member Author

SethTisue commented Feb 2, 2017

@vpetro's code is now in good enough shape that he was able to move a few tests over with only very small changes at the tops of the source files; stdout is redirected and .check contents compared to actual results.

this reduces the time to run a single small test from 7s (partest) to 2s (sbt/JUnit)

@SethTisue
Copy link
Member Author

SethTisue commented Feb 2, 2017

@sjrd the tests themselves remain unchanged except for adding package declarations and giving classes unique names. reading the .check file is handled by an external scaffold; I imagine you would simply use your own scaffold instead.

@som-snytt
Copy link

One idea for fastest a few years ago was to compile all pos tests together, with package names supplied synthetically. If compilation failed for some reason, and the test with the error succeeds standalone, try recursively to isolate the set with an error. (I never identified such a case.) Similarly neg and run, with classloader isolation for running.

So I like the idea of tests as snippets. Manually specifying package names is a bore.

I would object to the language that tests are in partest's "clutches". I distinctly remember reading and hating the instructions on how to use partest, as a learner. So I sympathize with resistance to complication. However, junit is not the be-all. The xunit philosophy is to write your own (simple) framework. Did I mention simple?

I intended to contribute a way to incorporate the .check with the test, but didn't follow thru. Something like SessionTest, which I am sorry for, because no mechanism for -update.

@SethTisue
Copy link
Member Author

SethTisue commented Feb 22, 2017

I would object to the language that tests are in partest's "clutches". [...] junit is not the be-all.

yes, a good reminder. while some tests are in partest's clutches and don't need to be, others are rather in partest's warm, loving embrace

I like the idea of tests as snippets

me too.

in the long run, I think partest itself can get smaller (since it doesn't need to do sbt's job anymore) and we should have fewer partest-based tests, but I don't have a grand vision of eliminating it entirely unless we can retain its advantages. it has some sweet features. you're right to mention --update-check, it's something I often wish I had in other contexts.

@SethTisue
Copy link
Member Author

SethTisue commented Mar 20, 2017

the Dotty folks have their own partest replacement project at scala/scala3#2125 (thanks @lrytz for the heads-up)

@SethTisue
Copy link
Member Author

The performance motivation is presumably a lot less strong now that partest doesn't fork for every test anymore.

Not sure whether to move this to "Backlog" or just close it — I still think it would be a nice improvement, but it's also understandable that we all (apparently) have felt we have too much else to do.

@SethTisue
Copy link
Member Author

something to keep in the back of our minds, but doesn't need an indefinitely-open issue.

@som-snytt
Copy link

Keeping the flame at scala/scala@00c1895

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants