Skip to content

Add FromTastyDecompilation decompilation blacklist #4645

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

Merged
merged 4 commits into from
Jul 25, 2018

Conversation

nicolasstucki
Copy link
Contributor

No description provided.

@nicolasstucki nicolasstucki self-assigned this Jun 11, 2018
@nicolasstucki nicolasstucki force-pushed the test-decompile-more-files branch 2 times, most recently from 9646b8b to 06ef486 Compare June 11, 2018 12:02
@nicolasstucki
Copy link
Contributor Author

With the current blacklist, it tests 1847 decompilations and 633 recompilations

@nicolasstucki nicolasstucki force-pushed the test-decompile-more-files branch 5 times, most recently from fa23ed6 to f633e98 Compare June 18, 2018 12:43
@nicolasstucki nicolasstucki force-pushed the test-decompile-more-files branch 9 times, most recently from c124b95 to 71439ab Compare June 19, 2018 17:26
@smarter
Copy link
Member

smarter commented Jun 19, 2018

We need some way to automatically update the blacklist, otherwise this will be impossible to maintain. I also suggest maintaining the list in a separate file, and still running the tests on the blacklisted files so we know when something starts to pass.

@nicolasstucki nicolasstucki force-pushed the test-decompile-more-files branch 10 times, most recently from 2096a5b to f6f8d04 Compare June 21, 2018 05:04
@nicolasstucki nicolasstucki force-pushed the test-decompile-more-files branch 9 times, most recently from d948875 to f745227 Compare July 20, 2018 11:07
@nicolasstucki nicolasstucki removed their assignment Jul 20, 2018
@nicolasstucki
Copy link
Contributor Author

This PR adds a blacklist for decompilation tests; this implies that we decompile all tests instead of only the ones that have an explicit XYZ.decompiled file. It also adds a whitelist for recompilation with all the tests that currently work, it is not a blacklist to avoid newly added tests to fail the decompilation tests.

@nicolasstucki
Copy link
Contributor Author

@allanrenucci could you review this now. Now it is not a wip anymore and it should be merged to have more regression tests.

Copy link
Contributor

@allanrenucci allanrenucci left a comment

Choose a reason for hiding this comment

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

Instead of having these blacklist/whitelist being passed as functions/Set, I would introduce a unique abstraction. Something like:

sealed trait FileFilter {
  def accept(file: String): Boolean
}

object FileFilter {
  def excludeAll(files: List[String]): FileFilter = new FileFilter {
    private[this] val blackList = files.toSet
    def accept(file: String) = !blackList.contains(file)
  }
  def includeAll(files: List[String]): FileFilter = new FileFilter {
    private[this] val whiteList = files.toSet
    def accept(file: String) = whiteList.contains(file)
  }
  object NoFilter extends FileFilter {
    def accept(file: String) = true
  }
}

@@ -0,0 +1,80 @@
// Constant(Symbol)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that supposed to be a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. These tests fail because they have a Constant(_) (as in Literal(Constant(_))) with a scala.Symbol inside.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't a comment start with #?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes

@nicolasstucki nicolasstucki force-pushed the test-decompile-more-files branch from e51e7ce to a951dd8 Compare July 24, 2018 13:39
@nicolasstucki nicolasstucki force-pushed the test-decompile-more-files branch from a951dd8 to 8d87b56 Compare July 24, 2018 14:59
@nicolasstucki
Copy link
Contributor Author

@allanrenucci, I added the FileFilter

Copy link
Contributor

@allanrenucci allanrenucci left a comment

Choose a reason for hiding this comment

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

I think I would merge ListOfSources.scala, FromTastySources.scala, StdLibSources.scala into a single file TestRessources or TestSources.

Otherwise LGTM

decompilationDir,
shouldDelete = true
)
}

class TastyCompilationTest(step1: CompilationTest, step2: CompilationTest, step3: CompilationTest,
recompilationBlacklist: Set[String], decompilationDir: String, shouldDelete: Boolean)(implicit testGroup: TestGroup) {
recompilationBlacklisted: FileFilter, decompilationDir: String, shouldDelete: Boolean)(implicit testGroup: TestGroup) {
Copy link
Contributor

Choose a reason for hiding this comment

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

recompilationBlacklisted?

@nicolasstucki nicolasstucki force-pushed the test-decompile-more-files branch from b622fd9 to 3c5d4ea Compare July 25, 2018 11:44
@nicolasstucki nicolasstucki merged commit 74e487b into scala:master Jul 25, 2018
@allanrenucci allanrenucci deleted the test-decompile-more-files branch July 25, 2018 15:42
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.

3 participants