Add junit test for text fields check #12057
Conversation
…erty.MULTILINE_TEXT properly
…t-test-for-text-fields-check merge upstream main
…rea creation, Check TextInputControl field
…t-test-for-text-fields-check merge upstream changes
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
You can check review dog's comments at the tab "Files changed" of your pull request.
There was a problem hiding this comment.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
You can check review dog's comments at the tab "Files changed" of your pull request.
There was a problem hiding this comment.
Your code currently does not meet JabRef's code guidelines.
We use OpenRewrite to ensure "modern" Java coding practices.
The issues found can be automatically fixed.
Please execute the gradle task rewriteRun, check the results, commit, and push.
You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".
| * b) Has an EditorTextArea object creation | ||
| */ | ||
| @Test | ||
| public void fieldEditorTextAreaTest() throws IOException { |
There was a problem hiding this comment.
More self-descriptive test name please. I think fieldEditorsMatchMultilineProperty could be a good name.
| */ | ||
| @Test | ||
| public void fieldEditorTextAreaTest() throws IOException { | ||
| // get all field editors and their properties in FieldEditors.java |
There was a problem hiding this comment.
Remove this comment - it is exactly the method name (which is good)
|
|
||
| CompilationUnit cu = PARSER.parse(filePath).getResult().orElse(null); | ||
| if (cu == null) { | ||
| throw new RuntimeException("Failed to analyze " + filePath); |
There was a problem hiding this comment.
No RuntimeExeptions in tests. Use normal Exceptions. And use throws clause.
NullPointerExceptions are also OK.
The text Failed to analyze is too generic. Is it because of a logic issue? The NullPointerException is more specific than a general failure
|
|
||
| try { | ||
| CompilationUnit cu = PARSER.parse(Paths.get(filePath)).getResult().orElse(null); | ||
| if (cu == null) { |
| } catch (IOException e) { | ||
| LOGGER.log(Level.SEVERE, "Error parsing file: " + filePath, e); | ||
| } |
There was a problem hiding this comment.
You are crafting a JUnit test - just let exceptions flow - no need to handle or log them.
However, it is green now. What do you mean by "unreliable".
No. Please keep as is. The statement
We can do as follow up. |
koppor
added
the
status: changes required
2. Rename the test method and file. 3. Use normal exceptions 4. Remove logger
|
I noticed that the holdEditorTextField method appears to be unused within the codebase. Should we consider removing it to simplify the code and improve maintainability? Unless there’s a planned usage or if it’s reserved for future features, removing unused methods could help keep the code clean and focused. |
It feels somewhat fragile since the check relies heavily on the current structure of |
I think we could keep it for now, as it might be useful once we can specifically check whether the FieldEditor should use a TextField. |
Where is this variable? IntelliJ doesn't find it.
|
Then, we can delete it. You can add a JavaDoc to the text just stating your text. You can be stronger: "This test is somewhat fragile ..." |
2. Optimize indentation and some comments
Closes #11939
This is an approach try to test if
FieldEditorhas the appropriate usage ofTextAreaorTextFieldby using Java Parser to analyzeFieldEditors.java. However, there are currently many limitations.This test does the following:
ifstatements inFieldEditors.getForFieldand create a map..javafile path of theFieldEditorclass (for example:src/.../UrlEditor.java)FieldPropertiesby analyzing theifstatement conditions (for example:[FieldProperty.EXTERNAL])propertiesincludeFieldProperty.MULTILINE_TEXT, then perform the following tests:FieldEditorFXand calls theestablishBindingmethod, get the name of the first parameter.TextInputControl.EditorTextAreacreation in this class.Limitations:
EditorTextFieldfield correctly because we cannot determine if theFieldEditorclass has multiple lines fromFieldEditors.java. For example, thePersonsEditorshould be able to useTextArea, but fromFieldEditors.javawe can't determine if thePersonEditorwill have theMULTILINE_TEXTproperty. In fact, none of theseFieldEditorclasses can be determined to have theMULTILINE_TEXTproperty if we only analyzeFieldEditors.java.Suggestions:
final boolean isMultiLinebut instead check if thefieldparameter holds theMULTILINE_TEXTproperty.TextAreaorTextField).Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)