Skip to content

Fix eight defects found offline, and unbreak the test suite - #263

Open
Pimptasty wants to merge 2 commits into
AIE-Guild:release/classicfrom
Pimptasty:pr/classic-fixes-2
Open

Fix eight defects found offline, and unbreak the test suite#263
Pimptasty wants to merge 2 commits into
AIE-Guild:release/classicfrom
Pimptasty:pr/classic-fixes-2

Conversation

@Pimptasty

Copy link
Copy Markdown
Contributor

Eight defects, each found by running the existing tests/ suite offline and writing a test for
what the code should do. Every fix has a regression test in this PR.

The suite on this branch is currently red — two errors in TestConfig. GwConfig:reload()
calls C_GuildInfo.GuildRoster() bare, while tests/TestConfig.lua asserts a fallback to the
global. The first commit restores the feature-detect so the two agree; it also protects a client
without the namespace, which raises there on the very first configuration load, before the bridge
comes up.

The rest

  • GW:v: could abort the entire configuration parse. The guard
    '^%d+%.%d+%.%d+%w*$' accepts 1.2.3beta, which is not a semantic version — a pre-release needs
    the -. semver() returns nothing, tostring() then raises with no argument, and load()
    unwinds before the channel, peer and officer directives on the following lines are read. One
    mistyped character in the Guild Information panel takes a whole confederation's configuration
    down. Now validated by parsing rather than by regex.

  • GwConfig:is_container() raised on every callself:GetGuildName(), which GwConfig does
    not define. The helper is gw.GetGuildName().

  • The Interface Options "Defaults" button raised. GwSettings:reset() requires an svtable,
    but GreenWallInterfaceFrame_SetDefaults calls it with none. Underneath sat a second defect:
    the body only assigned a default when the stored value was absent or invalid — a guard that
    belongs to initialize(), making reset() a no-op on any real store even when called correctly.

  • API handlers registered with '*' never received anything. The dispatcher tested whether the
    sending addon was named '*', but AddMessageHandler and SendMessage both assert the addon
    against C_AddOns.GetAddOnInfo, so no real sender can be. The wildcard belongs to the
    registration, as API.md documents.

  • The bridge channel was never hidden. GwChannel:join() scans GetChatWindowMessages() for
    the channel name, but that returns message group names (GUILD, SAY) — the channel list is
    GetChatWindowChannels(). Blizzard reads the two into RegisterForMessages and
    RegisterForChannels respectively, so a group name can never equal a channel name and the loop
    matches nothing. Had it matched it would have raised: ChatFrame_RemoveChannel is a deprecation
    alias for ChatFrameMixin.RemoveChannel, which takes the frame as self, and the call passed the
    frame's name.

  • Hold-down timers could stop firing silently, and leaked a frame per start. GwHoldDown:start()
    created its OnUpdate frame into a bare local; nothing else referenced it, because the handler
    takes frame as a parameter rather than capturing it. A timer that stops produces no error at
    all. Created once, kept on the instance, reused.

  • Three pre-release precedence defects in Lib/SemanticVersion.lua, in code that had never
    executed: a version with build metadata but no pre-release (1.0.0+20130313144700, valid per
    semver 2.0.0 §10) was rejected outright; the comparison was not antisymmetric, so
    1.0.0-alpha < 1.0.0-1 and 1.0.0-1 < 1.0.0-alpha were both true; and a shorter pre-release
    outranked a longer one, making 1.0.0-alpha rank above 1.0.0-alpha.1. TestSemanticVersion.lua
    walks the specification's own worked example.

    VERSION_MINOR goes 1 → 2, which is load-bearing rather than bookkeeping:
    LibStub:NewLibrary returns nil when oldminor >= minor, so at an unchanged minor this corrected
    copy would lose to any other add-on carrying an older copy that loaded first, and the fixes would
    ship without ever running.

Docs

gw.usage did not list mode, joindelay or refresh, so three working commands were
undiscoverable in game. README.md documented a stats command that has never existed — the slash
handler has no such branch.

Testing

All seven test files pass on this branch. luaunit is not vendored here, so:

LUA_PATH="./?.lua;./tests/?.lua;/path/to/luaunit/?.lua;;" lua tests/TestSemanticVersion.lua

The SemanticVersion tests fail 5/17 against the unfixed library and pass 17/17 with the fix.

Same change is offered against main, release/classic and release/classic-era.

Each of these fails silently: nothing raises where a user would see it, so the
symptom is a feature that quietly does nothing. Regression tests accompany every
one, in tests/.

Config.lua
  reload() called C_GuildInfo.GuildRoster() bare while tests/TestConfig.lua
  asserts a fallback to the global, so the suite is currently red on this branch.
  Restored the feature-detect, which also protects a client that lacks the
  namespace: it raises there on the first configuration load, before the bridge
  comes up.

  A GW:v: line the guard accepted but semver() rejects aborted the entire parse.
  '^%d+%.%d+%.%d+%w*$' allows '1.2.3beta', which is not a semantic version (a
  pre-release needs the '-'), so semver() returned nothing and tostring() raised
  with no argument -- unwinding out of load() before the channel, peer and
  officer directives on the following lines were read. One mistyped character in
  the Guild Information panel took the whole confederation's configuration down.
  Now validated by parsing; a bad version is logged and skipped.

  is_container() opened with self:GetGuildName(), which GwConfig does not define,
  so every call raised. The helper is gw.GetGuildName().

Settings.lua
  reset() required an svtable, but GreenWallInterfaceFrame_SetDefaults calls it
  with none -- the Interface Options "Defaults" button raised on the first key.
  Underneath, the body only assigned a default when the stored value was absent
  or invalid. That guard belongs to initialize(), which fills gaps; reset() must
  overwrite values that are present and valid, so it was a no-op on any real
  store even when called correctly.

API.lua
  A handler registered with '*' never received anything. The dispatcher tested
  whether the SENDING addon was named '*', but AddMessageHandler and SendMessage
  both assert the addon against C_AddOns.GetAddOnInfo, so no real sender can be.
  The wildcard belongs to the registration, as API.md documents.

Channel.lua
  The bridge channel was never hidden. join() scanned GetChatWindowMessages()
  for the channel name, but that returns message GROUP names (GUILD, SAY); the
  channel list is GetChatWindowChannels(). Blizzard reads the two into
  RegisterForMessages and RegisterForChannels respectively. A group name can
  never equal a channel name, so the loop matched nothing.

  Had it matched it would have raised: ChatFrame_RemoveChannel is a deprecation
  alias for ChatFrameMixin.RemoveChannel, which takes the frame as self, and the
  call passed the frame's NAME. Also iterates NUM_CHAT_WINDOWS rather than a
  hard-coded 10.

HoldDown.lua
  start() created its OnUpdate frame into a bare local. Nothing else referenced
  it -- the handler takes frame as a parameter rather than capturing it -- so
  the timer's survival depended on the client never reclaiming the frame, and a
  timer that stops firing produces no error at all. The flip side is a leak:
  refresh_channels restarts these for the whole session and each start created a
  new frame. Created once, kept on the instance, reused.

Lib/SemanticVersion.lua
  Three defects in pre-release comparison, none of which had ever executed:

  - A version with build metadata but no pre-release was rejected outright. The
    suffix pattern required a leading '-', so '1.0.0+20130313144700', valid per
    semver 2.0.0 section 10, parsed to nothing.
  - The comparison was not antisymmetric: alphanumeric-versus-numeric returned
    "less than" in both directions, so 1.0.0-alpha < 1.0.0-1 and 1.0.0-1 <
    1.0.0-alpha were both true and any ordering depended on argument order.
  - A shorter pre-release outranked a longer one, making 1.0.0-alpha rank above
    1.0.0-alpha.1 -- the reverse of the specification's own worked example, which
    tests/TestSemanticVersion.lua now walks end to end.

  VERSION_MINOR goes 1 -> 2, and that is load-bearing rather than bookkeeping:
  LibStub:NewLibrary returns nil when oldminor >= minor, so at an unchanged minor
  this corrected copy would lose to any other add-on carrying an older copy that
  happened to load first, and the fixes would ship without ever running.
gw.usage did not list mode, joindelay or refresh, so three working commands were
undiscoverable in game -- /gw help is the only place they are advertised.

README.md documented a "stats" command. There has never been one; the slash
handler has no such branch, so anyone following the documentation got "Unknown
command: stats".

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

Please submit each of the fixes as a separate pull request. With all eight fixes in one commit, it's difficult to isolate any regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants