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

Apply RemoveTestPrefix #10823

Merged
merged 7 commits into from
Jan 29, 2024
Merged

Apply RemoveTestPrefix #10823

merged 7 commits into from
Jan 29, 2024

Conversation

koppor
Copy link
Member

@koppor koppor commented Jan 24, 2024

Follow-up to #10797 and #10799.

This applies Remove test prefix from JUnit 5 tests and fixes the issues caused by openrewrite/rewrite-testing-frameworks#462 manually.

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@koppor koppor added the type: code-quality Issues related to code or architecture decisions label Jan 24, 2024
@koppor koppor changed the title Applye RemoveTestPrefix Apply RemoveTestPrefix Jan 24, 2024
@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 24, 2024
@koppor koppor enabled auto-merge January 25, 2024 10:34
@koppor
Copy link
Member Author

koppor commented Jan 25, 2024

Only FetcherTests fail, because of CiteSeer and Grobid (and others)

String formattedStr = formatter.format(specialChar);
assertEquals(expectedResult, formattedStr);
}

@Test
void testRTFCharacters() {
void rTFCharacters() {
Copy link
Member

Choose a reason for hiding this comment

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

,,,?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

one misspelled method name

@Siedlerchr
Copy link
Member

I don't like this refactoring, Even though it's not necessary, I find it easier to have the methods with test prefixed, helps it to find them

@calixtus
Copy link
Member

I have no strong opinion here, but it should be consistent everywhere. For now we have methods with and some without the prefix.

About the argument: I think the ide should help here. In intellij test classes have a green bg. But I don't know if this is what you mean?

What's more of interest to me in this matter is: what recommendation does junit make and how do the error messages look if junit produces some.

@koppor
Copy link
Member Author

koppor commented Jan 27, 2024

+1 for consistency. Therefore, I applied it.

It is also based on the experience by our friends at Moderne. Thus, I trust them --> https://docs.openrewrite.org/recipes/java/testing/cleanup/removetestprefix

Siedlerchr
Siedlerchr previously approved these changes Jan 27, 2024
@Siedlerchr
Copy link
Member

There is something wrong with the lobid fetcher:

Failed to map supported failure 'org.opentest4j.AssertionFailedError: expected: <Optional[Paper Title]> but was: <Optional.empty>' with mapper 'org.gradle.api.internal.tasks.testing.failure.mappers.OpenTestAssertionFailedMapper@151d4a62': Cannot invoke "Object.getClass()" because "expectedValue" is null

@koppor
Copy link
Member Author

koppor commented Jan 28, 2024

This PR did not touch the LOBID fetcher:

image

@koppor
Copy link
Member Author

koppor commented Jan 29, 2024

Copy link
Contributor

github-actions bot commented Jan 29, 2024

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

@koppor koppor added this pull request to the merge queue Jan 29, 2024
Merged via the queue into main with commit 936ab5f Jan 29, 2024
18 of 20 checks passed
@koppor koppor deleted the apply-RemoveTestPrefix branch January 29, 2024 09:32
Siedlerchr added a commit that referenced this pull request Feb 3, 2024
* upstream/main:
  Fix NPE when tab is close during load (#10841)
  Bump peter-evans/create-or-update-comment from 3 to 4 (#10838)
  Bump styfle/cancel-workflow-action from 0.12.0 to 0.12.1 (#10835)
  Bump lycheeverse/lychee-action from 1.9.1 to 1.9.3 (#10837)
  Bump gradle/gradle-build-action from 2 to 3 (#10836)
  Bump com.puppycrawl.tools:checkstyle from 10.12.7 to 10.13.0 (#10830)
  Bump org.mockito:mockito-core from 5.9.0 to 5.10.0 (#10834)
  Bump org.openrewrite.rewrite from 6.7.0 to 6.8.1 (#10833)
  Bump org.openrewrite.recipe:rewrite-recipe-bom from 2.6.2 to 2.6.3 (#10832)
  Apply RemoveTestPrefix (#10823)
  Update to jdk 21.0.2 (#10827)
  Fix escaping of slashes for TexShop (#10826)
@koppor koppor mentioned this pull request Mar 13, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: code-quality Issues related to code or architecture decisions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants