Skip to content

Add test for #620 #6647

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 3 commits into from
Jun 28, 2019
Merged

Add test for #620 #6647

merged 3 commits into from
Jun 28, 2019

Conversation

liufengyun
Copy link
Contributor

@liufengyun liufengyun commented Jun 9, 2019

The issue in #6642 shows that errors may be latent in seemingly
simple fixes if not tested, which can backlash us at a critical moment.

The issue in scala#6642 shows that errors may be latent in seemingly
simple fixes if not tested, which can backlash us at critical moment.
@allanrenucci
Copy link
Contributor

I am not sure this is a good idea. Printing a full compilation unit will be very fragile. Changes in type checking can break these tests. In #6310, we already disabled the decompilation tests that were very similar because there were too fragile.

Also note that we already have similar tests in PrinterTests. These tests are more fine grained and are less likely to break to unrelated changes. I think we should improve on them and expose a nicer API that require less boilerplate to define a test.

@liufengyun
Copy link
Contributor Author

liufengyun commented Jun 10, 2019

Thanks for raising the concern. I comment inline below:

I am not sure this is a good idea. Printing a full compilation unit will be very fragile. Changes in type checking can break these tests. In #6310, we already disabled the decompilation tests that were very similar because there were too fragile.

The difference from #6310 is that we don't blindly decompile Scala files in the test set and check the output. In contrast, the files we put in the printing test are carefully designed. It means:

  • we only have just a few carefully crafted Scala files (instead of thousands)
  • the code in the file is specially designed to test the printer, which should not contain code that is sensitive to type checking.

With icdiff (shown in the help message), compiler developer can easily see what has changed. That is how the trailing space problem is spotted in b544090, which is otherwise impossible to find.

Also note that we already have similar tests in PrinterTests. These tests are more fine grained and are less likely to break to unrelated changes. I think we should improve on them and expose a nicer API that requires less boilerplate to define a test.

I see two problems with that approach:

  • it's a pain to maintain the test and develop new ones
  • they are closely coupled with compiler internals, which suffers from the same fragibility problem, and assumably harder to fix once broken.

Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

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

The code itself LGTM

The remaining question is about stability of the test. As @liufengyun stated here we will handcraft the code to be as stable as possible while printing the desired part of the code. Hence we should not fall into the same amount of differences as with the decompiler tests.

I propose to merge it and see if we get into stability issues. If we do we can remove or modify the test infrastructure yet again.

@anatoliykmetyuk anatoliykmetyuk merged commit 3962d3c into scala:master Jun 28, 2019
@anatoliykmetyuk anatoliykmetyuk deleted the add-test-6642 branch June 28, 2019 13:31
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