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

Add a regression test for issue #10134 #9250

Closed
wants to merge 2 commits into from

Conversation

seratch
Copy link
Contributor

@seratch seratch commented Oct 18, 2020

This pull request adds a new partest suite verifying if the issue #10134 has been fixed.

@lrytz This pull request adds a test you mentioned in this comment:

Any volunteer for a regression test PR? I hope the infrastructure is in place to do scaladoc tests with mixed Java/Scala sources.

scala/bug#10134 (comment)

@scala-jenkins scala-jenkins added this to the 2.13.4 milestone Oct 18, 2020
@xuwei-k xuwei-k requested a review from lrytz October 18, 2020 05:14
@seratch
Copy link
Contributor Author

seratch commented Oct 18, 2020

Hmm, it seems the test doesn't correctly verify the behavior yet. I'm still learning the way to write valid partests for doc task.

@seratch seratch changed the title Add a regression test for issue #10134 WIP: Add a regression test for issue #10134 Oct 18, 2020
@seratch
Copy link
Contributor Author

seratch commented Oct 18, 2020

This pull request is not yet done. Currently, the test added by this PR fails as below.

# starting 1 test in run
!! 1 - run/t10134.scala                          [output differs]
% diff /Users/ksera/github/scala/test/scaladoc/run/t10134.check /Users/ksera/github/scala/test/scaladoc/run/t10134-run.log
@@ -1 +1,52 @@
-Done.
+newSource:1: error: value T10134_Row is not a member of object foo
+import foo.T10134_Row
+       ^
+newSource:2: error: not found: type T10134_Row
+@T10134_Row
+ ^
+java.lang.RuntimeException: Scaladoc Model Test ERROR: No universe generated!
+java.lang.RuntimeException: Scaladoc Model Test ERROR: No universe generated!
+	at scala.sys.package$.error(package.scala:27)
+	at scala.tools.partest.ScaladocModelTest.$anonfun$show$1(ScaladocModelTest.scala:75)
+	at scala.Option.getOrElse(Option.scala:201)
+	at scala.tools.partest.ScaladocModelTest.show(ScaladocModelTest.scala:75)
+	at scala.tools.partest.DirectTest.main(DirectTest.scala:111)

In the beginning, I assumed that I can easily add a new test suite by utilizing the existing test infra such as ScaladocModelTest. However, it seems I got it all wrong.

If I understand correctly, there is no existing test that loads multiple files from the resources directory yet. So, to add a test using two files (Scala source and Java source), I think we need to add a new test base class that covers the use case.

Although I may need some time for learning testing infra for scala project, I'm keen to complete this contribution. Let me know if you have suggestions on this.

@SethTisue SethTisue marked this pull request as draft October 19, 2020 17:32
@SethTisue SethTisue added the internal not resulting in user-visible changes (build changes, tests, internal cleanups) label Oct 19, 2020
@SethTisue SethTisue modified the milestones: 2.13.4, 2.13.5 Oct 19, 2020
@SethTisue
Copy link
Member

is there someone watching the repo who could offer @seratch some help here?

@SethTisue
Copy link
Member

SethTisue commented Dec 3, 2020

@seratch it's plausible that there simply isn't anybody around who knows this code. But I looked at ScaladocModelTest.scala myself just now and I agree, it seems it would need work to support this. I see there is also ScaladocJavaModelTest.scala, I assume you looked at that as well? That seems like it could be generalizable?

Lukas's request for a volunteer was based on the hope that it might be possible to add a test without too much trouble. It appears you've investigated to the point where it's clear that it isn't that easy. So perhaps it simply isn't worth the effort at this point, since the bug is already fixed, and since it seems unlikely that other bugs are going to be found that would be able to take advantage of any new testing infrastructure you would add.

I poked around a bit to see if I could find some other place where a test like this could go.

test/script-tests seems like it could possibly be suitable, but it also seems to be long-moribund, as per the README in that directory.

Support for mixed-Java-and-Scala compilation does exist in partest; there are plenty of tests in test/files that have both Scala and Java sources. But it doesn't seem that likely to me that it would be easier to add Scaladoc testing support there, rather than adding Java-and-Scala support to ScaladocModelTest/ScaladocJavaModelTest.

@SethTisue SethTisue changed the title WIP: Add a regression test for issue #10134 Add a regression test for issue #10134 Dec 3, 2020
@seratch
Copy link
Contributor Author

seratch commented Dec 4, 2020

@SethTisue Thanks for the response here! Yes, I agree that working on this may require some efforts and probably it's not a priority for the team. As I'm not planning to make additional changes to this PR, let me close this now. But if someone wants to reuse some of the changes, please feel free to do so!

Through this experience, I learned how to work with partests. I'm keen to work on other tasks when I have time 👋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal not resulting in user-visible changes (build changes, tests, internal cleanups)
Projects
None yet
3 participants