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 editor pattern test in BracketedPatternTest #9946

Merged
merged 7 commits into from
Jun 2, 2023

Conversation

chiao26168
Copy link
Contributor

Compulsory checks

The existing test class, org.jabref.logic.citationkeypattern.BracketedPatternTest.java, currently lacks coverage for editor patterns. To address this gap, I have added a new test method, testEditorFieldMarkers() to ensure that the editor pattern functions correctly.

During the execution of testEditorFieldMarkers(), one test case failed. The input provided was "'New', '[edtr3_1]', 'Isaac Newton'", and the expected output was 'New'. However, the actual output was null.

Screen Shot 2023-05-23 at 7 01 36 AM

(output of the failed test case)

The source of the error can be found in org.jabref.logic.citationkeypattern.BracketedPattern.
As stated in Customize the citation key generator, when the pattern matches "edtrN_M", the citation key should consist of the first N characters of the Mth editor's last name.

Here is a screenshot highlighting the problem in the source code:
Screen Shot 2023-05-23 at 7 10 10 AM

To fix this issue, the following return statement should be used

return authNofMth(editorList,
                            Integer.parseInt(nums[0]),
                            Integer.parseInt(nums[1]));

@Siedlerchr
Copy link
Member

Thank you very much! Please go ahead and fix this as well :)

Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 left a comment

Choose a reason for hiding this comment

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

Overall this looks very good, and I love the detailed description! I am slightly disturbed that the authNofMth error seems to have been here since 2017, so test cases for this are definitively needed 😱

Remaining

  • I am not sure about one of the test cases. Could you write down your thoughts regarding it, or if we have a similar test case in a different part of the code, reference it? I am hesitant to include it because I believe it might be a misuse of the editors' field, in which case all bets are off, and there isn't a right or wrong behavior of JabRef
  • Would you mind fixing the code style check complaints? It is much less important in a test case, but since it is only complaining about new lines
  • Since the behavior is changing, please add an entry to the change list, even if it is now the correct behavior as described in the documentation

@ThiloteE ThiloteE added keygenerator type: code-quality Issues related to code or architecture decisions labels May 27, 2023
@Siedlerchr
Copy link
Member

@chiao26168 can you please add a changelog entry?

@chiao26168
Copy link
Contributor Author

@chiao26168 can you please add a changelog entry?

Should I add two entries, one for adding the test case and the other for fixing the error?

@Siedlerchr
Copy link
Member

Changelog is intended for the end users so just one for the changed behavior/fix

Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 left a comment

Choose a reason for hiding this comment

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

LGTM! We greatly appreciate the detailed description, the missing test-cases, detection of a bug, and fixing of the bug 😅

CHANGELOG.md Outdated Show resolved Hide resolved
@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 1, 2023
Co-authored-by: Jonatan Asketorp <jonatan.asketorp@gmail.com>
@Siedlerchr Siedlerchr merged commit 6124f04 into JabRef:main Jun 2, 2023
@Siedlerchr
Copy link
Member

Thanks again for the tests and finding these bugs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keygenerator 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.

4 participants