openrewrite / rewrite-testing-frameworks

OpenRewrite recipes that perform common Java testing migration tasks.
Apache License 2.0
74 stars 67 forks source link

`TestsShouldNotBePublic` should reduce visibility if used elsewhere in Javadoc links #458

Open koppor opened 8 months ago

koppor commented 8 months ago

I applied TestsShouldNotBePublic to JabRef's code.

I get compile errors afterwards:

/home/runner/work/jabref/jabref/src/test/java/org/jabref/logic/bst/BstFunctionsTest.java:7: error: BstCaseChangersTest is not public in org.jabref.logic.bst.util; cannot be accessed from outside package import org.jabref.logic.bst.util.BstCaseChangersTest;

I know, this is bad software design. However, I think, rewriting should not introduce compile errors.

Refs https://github.com/JabRef/jabref/pull/10797

timtebeek commented 8 months ago

Oh wow; at first I thought this was a variant of the existing issue related to extended classes:

But in this case it seems the classes are only ever used for @link in Javadocs: https://github.com/JabRef/jabref/blob/899f5034ec4896aada28d3f79f391561d8843343/src/test/java/org/jabref/logic/bst/BstFunctionsTest.java#L7-L35

koppor commented 8 months ago

Not all:

org.jabref.logic.exporter.BibtexDatabaseWriterTest#roundtripWin1252HeaderKept

Path testFile = Path.of(BibtexImporterTest.class.getResource("encoding-windows-1252-with-header.bib").toURI());

And other similar things. I rewrote them by duplicating the test files and changing the access.


In the case of JavaDocs, the class reference should be changed to a fully qualified class referernce.

koppor commented 1 month ago

I have an edge case here:

interface SearchBasedFetcherCapabilityTest {
...
    @Test
    default void supportsYearSearch() throws Exception {
...
}
...
class SpringerFetcherTest implements SearchBasedFetcherCapabilityTest, PagedSearchFetcherTest {
...
    @Test
    @Disabled("401 as of 2024-08-18")
    @Override
    void supportsJournalSearch() {
    }
}

I think, I cannot easily rewrite that. We decided to have tests implementing interfaces ensuring consistency on compiler level and not on checker or manual review level.

timtebeek commented 1 month ago

I think it should be ready to skip methods annotated with override. And we already have a recipe that adds missing override annotations. Could be worth adding that cheap check here.

Would you be up for contributing that to the project? Happy to help guide of course.

timtebeek commented 1 month ago

I've explore overrides a bit in 671fe03accd83c78dbb407e9262f8208137cfa4f but saw no issue there. Reading your comment again I'm not sure if there even was an issue with the recipe. Could you clarify a bit what you're after?