Skip to content

Add editor pattern test in BracketedPatternTest#9946

Merged
Siedlerchr merged 7 commits into
JabRef:mainfrom
chiao26168:add-test-editor
Jun 2, 2023
Merged

Add editor pattern test in BracketedPatternTest#9946
Siedlerchr merged 7 commits into
JabRef:mainfrom
chiao26168:add-test-editor

Conversation

@chiao26168

@chiao26168 chiao26168 commented May 23, 2023

Copy link
Copy Markdown
Contributor 
### Compulsory 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](https://devdocs.jabref.org/): Is the information available and up to date? If not, I outlined it in this pull request.
- [ ] [Checked documentation](https://docs.jabref.org/): 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.

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

Siedlerchr commented May 23, 2023

Copy link
Copy Markdown
Member

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

k3KAW8Pnf7mkmdSMPHz27
k3KAW8Pnf7mkmdSMPHz27 requested changes May 24, 2023
View reviewed changes

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 left a comment •
edited
Loading

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.

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

Comment thread src/test/java/org/jabref/logic/citationkeypattern/BracketedPatternTest.java Outdated
@ThiloteE ThiloteE added component: citationkey-generator dev: code-quality Issues related to code or architecture decisions labels May 27, 2023
Siedlerchr
Siedlerchr reviewed May 31, 2023
View reviewed changes
Comment thread src/test/java/org/jabref/logic/citationkeypattern/BracketedPatternTest.java Outdated
@Siedlerchr

Siedlerchr commented May 31, 2023

Copy link
Copy Markdown
Member

@chiao26168 can you please add a changelog entry?

@chiao26168

chiao26168 commented May 31, 2023

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

Siedlerchr commented May 31, 2023

Copy link
Copy Markdown
Member

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

k3KAW8Pnf7mkmdSMPHz27
k3KAW8Pnf7mkmdSMPHz27 previously approved these changes Jun 1, 2023
View reviewed changes

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

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

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

Siedlerchr commented Jun 2, 2023

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

Reviewers

@Siedlerchr Siedlerchr Siedlerchr approved these changes

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 left review comments

Assignees

No one assigned

Labels

component: citationkey-generator 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chiao26168 @Siedlerchr @k3KAW8Pnf7mkmdSMPHz27 @ThiloteE