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

Clean up tests #165

Merged
merged 14 commits into from
May 27, 2017
Merged

Clean up tests #165

merged 14 commits into from
May 27, 2017

Conversation

olafurpg
Copy link
Contributor

@olafurpg olafurpg commented May 25, 2017

The scalafix repo has accumulated a lot of successful and not-so-successful experiments over the past year. This PR cleans up what I found to be not-so-successful around our testing infrastructure an imports management:

  • unit tests in .source files. When I started scalafix is was only syntactic like scalafmt so I ported the test infrastructure from scalafmt. Now that scalafix does semantic rewrites, I feel that the .source files have a lot of limitation: no IntelliJ support, no syntax highlighting, bad compiler error message, dependency on scala-compiler, fragile interface with ScalahostPipeline and other compiler APIs. Basically, the test suite has become a half-baked built tool. This PR removes all of that in favor of using sbt for compiling/building the Semantic API, see 9beff13
  • OrganizeImports that was trying to do too much. OrganizeImports was implemented before Symbols appeared and therefore had a lot of inherent design issues. I ported the implementation to something less ambitious: no sorting, no expanding relative imports, no grouping, no ImportsConfig. This current implementation is far from perfect, but it won't mess up with people's imports as much.

Also, it always bugged me that misformatted file errors blocked tests. This
change makes it so that scalafmt is only run on a single entry in the
build matrix
- scalafix-testkit no longer interfaces with the ScalahostPipeline API.
  Instead, we use sbt-scalahost to build a mirror and buildinfo to
  generate the necessary mappings between original and expected code.
  This results in a cleaner test setup:
  - no more .source files without syntax highlighting and auto-completion.
  - less duplication wrapping every unit test in `object a { }`
  - cleaner compiler errors in input/output projects
  - easier to use custom classpath for single test suite, just add
    a new project!
- Remove old organize imports. OrganizeImports was implemented before
  Symbols came and the current implementation is weird adaptation from
  the old ScalafixMirror that no longer exists. Instead of keeping this
  cruft, this commit removes the import patches with less ambitious
  patch implementation that are still somewhat useful.
- Remove more legacy.
- Support SKIP and ONLY.
- Throw error at end of test suite if some test is marked as ONLY
@olafurpg
Copy link
Contributor Author

cc/ @ShaneDelmore scalafix-testkit just got a whole lot simpler, since it no longer uses .source files. Files are written in regular .scala files instead with full IntelliJ support. This change also buys us sbt-zinc incremental compilation for free 😄 The only downside is that code for original/expected pairs are no longer next to each other in the same file. However, the diffs for failed tests are still just as pretty. I believe the benefits outweigh the downsides.

@olafurpg olafurpg force-pushed the testkit branch 3 times, most recently from 92119f9 to 1142a90 Compare May 25, 2017 22:26
Copy link
Contributor

@xeno-by xeno-by left a comment

Choose a reason for hiding this comment

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

I like looking at pull requests that remove bunches of code! ❤️

implicit class XtensionSeqPatch(patches: Seq[Patch]) {
def asPatch: Patch = Patch.fromSeq(patches)
implicit class XtensionSeqPatch(patches: Iterable[Patch]) {
def asPatch: Patch = Patch.fromIterable(patches)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the motivation behind Iterable? I wonder what use cases you found for this more general type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iirc, I'm using views on Tokens to build patches. Views are not Seq[]

@@ -0,0 +1,69 @@
/*
rewrites = ExplicitReturnTypes
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@olafurpg olafurpg mentioned this pull request May 26, 2017
Travis CI kills the jobs. Those tests only run a subset of rewrites
anyways. Those tests are incredibly handy for manual testing, less so
for automated CI on every PR commit.
bjaglin pushed a commit to liancheng/scalafix that referenced this pull request May 23, 2023
Bumps [coursier/cache-action](https://github.com/coursier/cache-action) from v5 to v6.
- [Release notes](https://github.com/coursier/cache-action/releases)
- [Commits](coursier/cache-action@v5...730a6a4)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

2 participants