fix: clear the warning-sink ThreadLocal and drop dead test state - #146
Merged
Conversation
TypeConverter's ACTIVE ThreadLocal was never removed, so the last sink stayed referenced for the life of the thread -- a slow leak on a server thread pool. It now starts unset, warn() falls back to LOG, and restoring a null sink clears the entry. Also removes an unused import, the unused namespace on the NamespacedKey test double, and the unused name on Sounds. StaticFieldTestClass.PRIVATE_INSTANCE is kept and annotated instead: it is read only by reflection and is the sole case covering the canAccess(null) == false branch, so removing it drops getStaticFieldName to 7/8.
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
NamespacedKey takes a namespace and a key because the real Bukkit class does; the namespace is now exposed through getNamespace() the same way, rather than the field being deleted for being unread. Sounds likewise keeps its named instances and static factory, with the getName() accessor StaticFieldTestClass already uses. Fidelity to the API under reflection is the point of these doubles, so unused-field warnings on them are answered by using the field, not by reshaping the class.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Fixes the 5 Sonar issues still open on master after the
setAccessibleones were accepted. 263 tests green, branch coverage unchanged at 303/303.The real one
TypeConverter:31—Call "remove()" on "ACTIVE"ACTIVEwas aThreadLocalcreated withwithInitial(...)and only everset, neverremoved.YamlFileInterface.load(Object)installs the plugin's logger and restores the previous value in afinally— but "restoring" wrote the default back rather than clearing the entry, so every thread that ever loaded a config kept a sink referenced for its lifetime. On a server thread pool those threads outlive the load.It now starts unset,
warn()falls back toLOGwhen nothing is installed, and restoring anullsink callsremove():The existing
finally { pushSink(prev) }call site needed no change —previs nownullon the first install, which clears the entry.Dead state removed
LeniencyTest:7— unusedjava.nio.file.Filesimport.NamespacedKey:7— thensfield was assigned and never read. The double exists sotryFormatAsKeyedcan callkey()thenvalue(); the namespace was never part of that. Field and constructor parameter dropped, and the single call site inSoundImplupdated.Sounds:9— same story. The tests only compare identity (Sounds.MY_SOUND_ROCKS), so thenamefield, thegetSoundfactory and the@NotNullimport all went. The class deliberately keeps no publicStringconstructor and notoString()override — that is what makesYamlWriterfall through to static-field matching — so the comment now records it.Kept on purpose
StaticFieldTestClass:11—PRIVATE_INSTANCEThis one is a false positive worth explaining rather than obeying. The field is read only by reflection, from
YamlWriter#getStaticFieldName, and it is the only case exercising thefield.canAccess(null) == falsebranch.Measured rather than assumed — deleting it:
So it stays, with
@SuppressWarnings("unused")and a comment recording why, which is also what stops the issue reappearing.On the coverage numbers
A local build shows 2 uncovered lines in
YamlWrapperFactory(the v1-vs-v2 selection). That is not a regression and not related to this change: only one SnakeYAML version can take that branch per run. Verified the union acrossreport-1.14.xmlandreport-2.4.xmlleaves no uncovered lines, which is why Sonar — aggregating all four reports — reports 100%.