Skip to content

Apply RemoveTestPrefix#10823

Merged
koppor merged 7 commits into
mainfrom
apply-RemoveTestPrefix
Jan 29, 2024
Merged

Apply RemoveTestPrefix#10823
koppor merged 7 commits into
mainfrom
apply-RemoveTestPrefix

Conversation

@koppor

@koppor koppor commented Jan 24, 2024

Copy link
Copy Markdown
Member

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 dev: 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

koppor commented Jan 25, 2024

Copy link
Copy Markdown
Member Author

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


@Test
void testRTFCharacters() {
void rTFCharacters() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

,,,?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@calixtus calixtus left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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

koppor commented Jan 27, 2024

Copy link
Copy Markdown
Member Author

+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
Copy Markdown
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

koppor commented Jan 28, 2024

Copy link
Copy Markdown
Member Author

This PR did not touch the LOBID fetcher:

image

@koppor

koppor commented Jan 29, 2024

Copy link
Copy Markdown
Member Author

@github-actions

github-actions Bot commented Jan 29, 2024

Copy link
Copy Markdown
Contributor

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
@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

dev: code-quality Issues related to code or architecture decisions status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants