simplify state sorting status unit test - avoid duplicated code in HtmlTreeBuilderStateTest.java #1157
Conversation
|
Just found that HtmlTreeBuilderTest and TokeniserStateTest have similar structure and requirement on newly added string array or char array. It's not a big deal but I think extract these kinds of tests would be beneficial. @jhy What you say?
|
| List<String[]> arrays = new ArrayList<>(fields.length); | ||
| for (Field field : fields) { | ||
| int modifiers = field.getModifiers(); | ||
| if (Modifier.isStatic(modifiers) && Modifier.isFinal(modifiers) && field.getType() == String[].class) { |
There was a problem hiding this comment.
My main concern with changing the test to use reflection like this is that the test could, now or in the future, accidentally be skipping fields that need to be tested. E.g. what if down the line we add a constant and for some reason don't mark it final? (I know a bit of a forced example.)
I think if we also add an explicit test that there were n arrays matched it would be more explicit, and can keep a warning in the source to review the test.
Yes, would be good to update the other tests like this, and share a common piece of reflection code.
|
Hi Jonathan, since this is not a big deal, and the minor copy+paste is more straight forward for unit test, I would like to drawback my PR and you may balance correspondingly. Thank you for your reply and consideration. |
In test file HtmlTreeBuilderStateTest it duplicated tags to search through and there is also a special hint to remind maintainer to synchronize these arrays whenever there is any newly added.
So I made a small improvement to access these arrays via reflection and I tested that the uni test execution time just slow down very little as I executed tests before and after several times.