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

Refactored UnicodeToLatexFormatterTest #7699

Closed
wants to merge 2 commits into from
Closed

Refactored UnicodeToLatexFormatterTest #7699

wants to merge 2 commits into from

Conversation

ningxie1991
Copy link
Contributor

This pull request contributes to issue #6207, which is to add more unit tests or improve existing ones. I used Pitest to compute the line and mutation coverage. The empty string input was missing in the test, as well as the boundary case test for high code point unicode inputs.

  • 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 documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Comment on lines 23 to 28
Arguments.of("", ""), // empty string input
Arguments.of("abc", "abc"), // non unicode input
Arguments.of("\u00E5\u00E4\u00F6", "{{\\aa}}{\\\"{a}}{\\\"{o}}"), // multiple unicodes input
Arguments.of("\u0081", ""), // high code point unicode, boundary case: cp = 129
Arguments.of("\u0080", "€"), // high code point unicode, boundary case: cp = 128 < 129
Arguments.of(new UnicodeToLatexFormatter().getExampleInput(), "M{\\\"{o}}nch")
Copy link
Member

Choose a reason for hiding this comment

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

Please switch parameters to comply with assertEquals semantics: first the expected, then the input value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code is updated as suggested. :)

Switched the position of argument and expected output to comply with unit test convention
@ningxie1991 ningxie1991 requested a review from koppor May 4, 2021 08:45
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

See checkstyle/reviewdog. Otherwise is good

@koppor
Copy link
Member

koppor commented May 6, 2021

@ningxie1991 Maybe, we need to guide you how to read the checkers of GitHub. I marked the required tests:

grafik

They are all required. If they are green, they are good to go. If they are yellow, one has to wait (and can do nothing currently). If they are red, one has to work on them. Thus, please work on checkstyle. In case you followed our guidelines to setup the local workspace, Ctrl+Alt+L should reformat the code properly.

@Siedlerchr Siedlerchr added the status: changes required Pull requests that are not yet complete label May 10, 2021
@Siedlerchr Siedlerchr closed this May 17, 2021
@koppor
Copy link
Member

koppor commented May 17, 2021

We closed the PR, because there was no activity and the changes were rather small.

In case you are still interested in contributing, please try to work on another issue.

@Siedlerchr
Copy link
Member

I fixed the checkstlye issue and merged it 0c8662e

@ningxie1991
Copy link
Contributor Author

We closed the PR, because there was no activity and the changes were rather small.

In case you are still interested in contributing, please try to work on another issue.

Really sorry that I lost track of this pull request. I will make sure required tests pass in future pull request.

@ningxie1991
Copy link
Contributor Author

I fixed the checkstlye issue and merged it 0c8662e

Thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: changes required Pull requests that are not yet complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants