301 Days (redux)

Day 80 - Code Dissatisfaction

by Joe Threeohone

In which we question a lot of what we’ve written.

Test Every Day

Bless

The Bless potion just combined a few different effects and is pretty trivial (if time-consuming) to test:

features/player_effects.featurelink
696
697
698
699
700
701
702
703
704
705
706
707
708
709
710
711
712
713
714
715

Scenario: Bless potion has desired effect
    Given I use the "minimal" database as-is
    And I add player and character "TestBP01"
    And I set the character's "dexterity" stat to "10"
    And I set the character's "constitution" stat to "8"
    And I create an "Bless" potion of amount "0" and duration "6"
    And I put the item in the character's left hand
    And I start the server and play
    When I open and drink "bottle"
    And I have a "dexterity" stat of "10"
    And I have a temporary "dexterity" stat of "1"
    And I have a "constitution" stat of "8"
    And I have a temporary "constitution" stat of "1"
    And I have a shielding of "1"
    And I rest
    And I have a "dexterity" stat of "10"
    And I have a temporary "dexterity" stat of "0"
    And I have a "constitution" stat of "8"
    And I have a temporary "constitution" stat of "0"
    And I have a shielding of "0"

Wizard_Eye

This is a lot more interesting. From the server code, in CreateCharacterEffect:

DragonsSpine/GameSystems/Effects/Effect.cslink
831
832
833
834
835
836
837
838
839
840
841
842
843
844
845
846
847
848
849
850
851
852
853
854
855

    case EffectType.Wizard_Eye:
        target.MyClone = target.CloneCharacter();
        target.MyClone.IsPC = false;
        Effect.CreateCharacterEffect(EffectType.Wizard_Eye, 0, target.MyClone, duration, target);
        Effect.CreateCharacterEffect(EffectType.Hide_In_Shadows, 0, target, -1, target);
        target.MyClone.CurrentCell = target.CurrentCell;
        target.MyClone.AddToWorld();
        if (target.BaseProfession == Character.ClassType.Thief)
            target.Name = "rat";
        else
            target.Name = "toad";
        Character.SetCharacterVisualKey(target);
        target.RightHand = null;
        target.LeftHand = null;
        target.Hits = 3;
        target.HitsMax = 3;
        target.HitsAdjustment = 0;
        target.Stamina = 3;
        target.StaminaMax = 3;
        target.StaminaAdjustment = 0;
        target.Mana = 0;
        target.ManaMax = 0;
        target.ManaAdjustment = 0;
        target.animal = true;
        break;

So it creates an NPC clone of the target, puts the same wizard eye effect on the clone, hides the target in shadows indefinitely, brings the clone into the world at the same location as the target, and changes the target to a toad (or rat if they are a thief).

That’s a bunch of stuff to test; let’s try something simple first:

features/player_effects.featurelink
749
750
751
752
753
754
755
756
757
758
759

Scenario: Wizard Eye potion results in OOBE
    Given I use the "minimal" database as-is
    And I add player and character "TestHS01"
    And I create an "Wizard_Eye" potion of amount "0" and duration "2"
    And I put the item in the character's left hand
    And I start the server and play
    When I open and drink "bottle"
    Then I saw myself
    And I see my doppelganger in a trance
    And I rest
    And I did not see myself
features/steps/character_steps.rblink
396
397
398
399
400
401
402
403
404
405
406
407
408
409

Then(/^I saw myself$/) do
    expect(@last_resp).to match(/\b#{@user[:char_name]}\b/)
end

Then(/^I did not see myself$/) do
    expect(@last_resp).to_not match(/\b#{@user[:char_name]}\b/)
end

Then(/^I see my doppelganger in a trance$/) do
  # The gsub is in case this text is split between lines
  resp = telnet_command(@connection, "look at #{@user[:char_name]}", / ->/).gsub(/\s+/, ' ')
  waited_for = /\b#{@user[:char_name]} is in a trance./i
  expect(resp).to match(waited_for)
end

It works, although I’m starting to get uncomfortable with our lack of parsing on the output: there’s no distinction between the map, the character list, flavor text, and stats bar. False positives are definitely possible where we mistake text in one for text in another.

While we contemplate that, let’s go one step further:

features/player_effects.featurelink
762
763
764
765
766
767
768
769
770
771
772
773

Scenario: Wizard Eye potion effect visible by others
    Given I use the "minimal" database as-is
    And I add player and character "TestHS01"
    And I create a "Wizard_Eye" potion of amount "0" and duration "50"
    And I put the item in the character's left hand
    And I start the server and play
    When I open and drink "bottle"
    And I add player and character "TestHS02"
    And I log on as "TestHS02"
    And I enter the game
    Then I saw a "toad"
    And I see "TestHS01_name" in a trance

And something else I’m not comfortable with: the way we handle multiple characters being used in a test. Interesting player interaction is going to take a lot of work; so let’s stop there for now.

Tackle a TODO every other day

Empty Hands

features/config_file.featurelink
266
267
268
269

## RequireMakeRecallReagent
# TODO - empty hands between tests, use database as-is
Scenario: RequireMakeRecallReagent False, thief can make recall ring from nothing
    Given I use the "minimal" database

So I add a hook and tag the tests:

features/lib/hooks.rblink
40
41
42

After('@clear_player_hands') do
  clear_player_hands
end
features/lib/db/player.rblink
166
167
168
169
170
171
172
173
174

def clear_player_hands
  client = connect_to_db(@server_database)
  query = "UPDATE [dbo].[PlayerHeld] SET \
    itemID = 0, attunedID = 0, special = ''"
  debug_msg query
  result = client.execute(query)
  affected_rows = result.do
  debug_msg "Cleared #{affected_rows} player hands"
end

But then I realized that I already had a hook for clearing all of the player stuff in the database, and it’d be far simpler to just use that after every test than worry about whether I’d altered hands or effects or inventory or whatever.

features/lib/hooks.rblink
50
51
52
53
54

After('@db_cleanup') do
  delete_all_test_accounts
  delete_all_test_players
  clear_log
end

New Player

Rubocop’s been providing me with lots of additional TODOs. It didn’t like my db_insert_player method, and neither did I:

features/lib/db/player.rblink
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51

def db_insert_player(db_hash)
  # anything in the hash overrides defaults
  player = DEFAULT_PLAYER.merge(db_hash)
  client = connect_to_db(@server_database)
  query = "INSERT INTO [#{@server_database}].[dbo].[Player] ([notes], [accountID], [account], \
    [name], [gender], [race], [classFullName], [classType], [visualKey], [alignment], [confRoom], \
    [impLevel], \
    [ancestor], [ancestorID], [facet], [land], [map], [xCord], [yCord], [zCord], [dirPointer], \
    [stunned], [floating], [dead], [fighterSpecialization], [level], [exp], [hits], [hitsMax], \
    [hitsAdjustment], [hitsDoctored], [stamina], [stamLeft], [staminaAdjustment], [mana], \
    [manaMax], [manaAdjustment], [age], [roundsPlayed], [numKills], [numDeaths], [bankGold], \
    [strength], [dexterity], [intelligence], [wisdom], [constitution], [charisma], [strengthAdd], \
    [dexterityAdd], [birthday], [lastOnline], [deleteDate], [currentKarma], [lifetimeKarma], \
    [lifetimeMarks], [pvpKills], [pvpDeaths], [playersKilled], [playersFlagged], [UW_hitsMax], \
    [UW_hitsAdjustment], [UW_staminaMax], [UW_staminaAdjustment], [UW_manaMax], \
    [UW_manaAdjustment], [UW_intestines], [UW_liver], [UW_lungs], [UW_stomach]) \
    VALUES (N'', #{player[:accountID]}, \
    N'#{player[:account]}', \
    N'#{player[:name]}', \
    1, N'Illyria', N'Fighter', \
    N'Fighter', N'male_fighter_pc_brown', 1, 0, #{player[:impLevel]}, 0, 0, 0, 0, 0, 42, 27, 0, \
    N'v', 0, 3, 0, N'None', 4, 4996, 36, 36, 0, 0, 10, 10, 0, 0, 0, 0, 806, 806, 8, \
    0, 0, 17, 18, 10, 17, 8, 18, 1, 1, CAST(0x0000A4FC0001D163 AS DateTime), \
    CAST(0x0000A4FC001A5F6A AS DateTime), CAST(0x0000979200000000 AS DateTime), \
    0, 0, 0, 0, 0, N'', N'', 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)"
  result = client.execute(query)
  affected_rows = result.do
  fail "Unable to insert player!" if 1 != affected_rows
  add_spell_slots_for_player(get_player_id(player[:name]))
  add_hand_slots_for_player(get_player_id(player[:name]))
  add_effect_slots_for_player(get_player_id(player[:name]))
  add_skills_for_player(get_player_id(player[:name]))
end

So we move all of the defaults into the DEFAULT_PLAYER hash:

features/lib/db/player.rblink
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20

DEFAULT_PLAYER = {
  accountID: 0, account: 'no_name', name: 'no_name', gender: 1, race: 'Illyria',
  classFullName: 'Fighter', classType: 'Fighter', visualKey: 'male_fighter_pc_brown', alignment: 1,
  confRoom: 0, impLevel: 0, ancestor: 0, ancestorID: 0, facet: 0, land: 0, map: 0, xCord: 42,
  yCord: 27, zCord: 0, dirPointer: 'v', stunned: 0, floating: 3, dead: 0,
  fighterSpecialization: 'None', level: 4, exp: 4996, hits: 36, hitsMax: 36, hitsAdjustment: 0,
  hitsDoctored: 0, stamina: 10, stamLeft: 10, staminaAdjustment: 0, mana: 0, manaMax: 0,
  manaAdjustment: 0, age: 806, roundsPlayed: 806, numKills: 8, numDeaths: 0, bankGold: 0,
  strength: 17, dexterity: 18, intelligence: 10, wisdom: 17, constitution: 8, charisma: 18,
  strengthAdd: 1, dexterityAdd: 1, birthday: :last_week, lastOnline: :yesterday,
  deleteDate: :last_year, currentKarma: 0, lifetimeKarma: 0, lifetimeMarks: 0, pvpKills: 0,
  pvpDeaths: 0, playersKilled: '', playersFlagged: '', UW_hitsMax: 0, UW_hitsAdjustment: 0,
  UW_staminaMax: 0, UW_staminaAdjustment: 0, UW_manaMax: 0, UW_manaAdjustment: 0,
  UW_intestines: 0, UW_liver: 0, UW_lungs: 0, UW_stomach: 0
}.freeze

And then we can make db_insert_player nice and Rubocop-friendly:

features/lib/db/player.rblink
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52

def db_insert_player(db_hash)
  debug_msg player = DEFAULT_PLAYER.merge(db_hash) # anything in the hash overrides defaults
  client = connect_to_db(@server_database)
  debug_msg query = db_insert_from_hash('Player', player)
  result = client.execute(query)
  affected_rows = result.do
  fail 'Unable to insert player!' if 1 != affected_rows
  add_misc_to_player(get_player_id(player[:name]))
end

def add_misc_to_player(player_id)
  add_spell_slots_for_player(player_id)
  add_hand_slots_for_player(player_id)
  add_effect_slots_for_player(player_id)
  add_skills_for_player(player_id)
end

Fix a Bug in the Legacy Code Every Fifth Day

features/player_effects.featurelink
57
58
59
60
61
62
63
64
65
66
67
68

## Ale

# bug - No indication of the burp is sent to the player themselves.
@bug
Scenario: Ale effect gives the desired output - same player, beginning
    Given I use the "minimal" database as-is
    And I add player and character "TestAle01"
    And I give the character an "ale" effect of "999" for "2" turns
    And the server is started
    When I log on as "TestAle01"
    And I enter the game
    And I saw a message "You burp loudly."

I can certainly add a burp to the server code (and I did, briefly); but that’s not where it belongs, is it?

EntireDB-minimal.sqllink
603
604
605
606
607

CREATE TABLE [dbo].[CatalogItem](
  [catalogID] [int] IDENTITY(1,1) NOT NULL,
  [notes] [nvarchar](255) NULL,
  [combatAdds] [int] NOT NULL,
  [itemID] [int] NOT NULL,

…

EntireDB-minimal.sqllink
649
650
651
652

  [drinkDesc] [varchar](255) NOT NULL,
  [fluidDesc] [varchar](255) NOT NULL
) ON [PRIMARY] TEXTIMAGE_ON [PRIMARY]
GO

The drinkDesc is what’s intended to be shown to the player themselves when an item is drunk. So let’s fix the test, not the server:

features/player_effects.featurelink
59
60
61
62
63
64
65
66
67

Scenario: Ale effect gives the desired output - same player, beginning
    Given I use the "minimal" database as-is
    And I add player and character "TestAle01"
    And I create an "Ale" potion of amount "0" and duration "2"
    And I give the item a "drinkDesc" of "BURP!"
    And I put the item in the character's left hand
    And I start the server and play
    When I open and drink "bottle"
    And I saw a message "BURP!"
features/steps/character_steps.rblink
54
55
56
57

Given(/^I give the item (a|an) "([^"]*)" of "([^"]*)"$/) do |_aan, field, value|
  new_hash = Hash[field.to_sym, value]
  @item_id = create_item_from_blank({ itemID: @item_id }, new_hash)  
end

I use my nifty create_item_from_blank method, using the existing item as the “blank”, to make a new item with a changed field.

That works fine, but when I fix the player-in-the-same-cell test:

features/player_effects.featurelink
80
81
82
83
84
85
86
87
88
89
90
91
92
93

Scenario: Ale effect gives the desired output - other player in same cell
    Given I use the "minimal" database as-is
    And I add player and character "TestAle01"
    And I create an "Ale" potion of amount "0" and duration "2"
    And I give the item a "drinkDesc" of "BURP!"
    And I put the item in the character's left hand
    And I add player and character "TestAle02"
    And the server is started
    When I log on as "TestAle02"
    And I enter the game
    And I rest
    And I cause "TestAle01" to log on and enter the game
    And I cause the secondary player to "open bottle; drink bottle"
    Then within "4" turns I see the message "TestAle01_name burps loudly."

I still don’t see it. Maybe there really is a bug to fix on the server side!

The effect code calls uses SendToAllInSight, so let’s add a bunch of debug to that:

DragonsSpine/GameObjects/GameLiving/Character/Character.cslink
2883
2884
2885
2886
2887
2888
2889
2890
2891
2892
2893
2894
2895
2896
2897
2898
2899
2900
2901
2902
2903
2904
2905
2906
2907
2908
2909
2910
2911
2912
2913
2914
2915
2916
2917
2918
2919
2920
2921
2922
2923
2924
2925
2926
2927
2928
2929
2930
2931
2932

public void SendToAllInSight(string text) // send text to everyone in sight, except this character
{
    int bitcount = 0;
    Cell curCell = null;

    for (int ypos = -3; ypos <= 3; ypos++)
    {
        for (int xpos = -3; xpos <= 3; xpos++)
        {
            Utils.Log("SendToAllInSight from " + this.Name + " considering relative cell " + xpos + ", " + ypos + "(bitcount " + bitcount + ")", Utils.LogType.Unknown);
            if (Cell.CellRequestInBounds(this.CurrentCell, xpos, ypos))
            {
                Utils.Log("SendToAllInSight from " + this.Name + " considers relative cell " + xpos + ", " + ypos + " in bounds.", Utils.LogType.Unknown);
                if (this.CurrentCell.visCells[bitcount]) // check the PC list, and char list of the cell
                {
                    Utils.Log("SendToAllInSight from " + this.Name + " considers relative cell " + xpos + ", " + ypos + " visible?", Utils.LogType.Unknown);
                    curCell = Cell.GetCell(this.FacetID, this.LandID, this.MapID, this.X + xpos, this.Y + ypos, this.Z);

                    if (curCell != null)
                    {
                        Utils.Log("SendToAllInSight from " + this.Name + " considers relative cell " + xpos + ", " + ypos + " non-null?", Utils.LogType.Unknown);
                        foreach (Character chr in curCell.Characters) // search for the character in the charlist of the cell
                        {
                            Utils.Log("SendToAllInSight from " + this.Name + " considering " + chr.Name + ".", Utils.LogType.Unknown);
                            if (chr != this && chr.IsPC && this.IsPC) // players sending messages to other players
                            {
                                Utils.Log("SendToAllInSight from " + this.Name + " considering " + chr.Name + " further.", Utils.LogType.Unknown);
                                if (Array.IndexOf(chr.ignoreList, this.PlayerID) == -1) // ignore list
                                {
                                    Utils.Log("SendToAllInSight from " + this.Name + " sending \"" + text + "\" to " + chr.Name + ".", Utils.LogType.Unknown);
                                    if (chr.filterProfanity) // profanity filter
                                    {
                                        text = Conference.FilterProfanity(text);
                                    }
                                    Utils.Log("SendToAllInSight from " + this.Name + " sending (filtered) \"" + text + "\" to " + chr.Name + ".", Utils.LogType.Unknown);
                                    chr.WriteToDisplay(text);
                                }
                            }
                            else if (chr != this && chr.IsPC && !this.IsPC) // non players sending messages to other players
                            {
                                chr.WriteToDisplay(text);
                            }
                        }//end foreach
                    }
                }
                bitcount++;
            }
        }
    }
}

After enabling all of the debug, we can see that the bitcount index into the visCells array is behaving oddly.

12/23/2016 10:09:13 PM: {Unknown} SendToAllInSight from TestAle02_name considering relative cell -3, -3(bitcount 0)
12/23/2016 10:09:13 PM: {Unknown} SendToAllInSight from TestAle02_name considering relative cell -2, -3(bitcount 0)
12/23/2016 10:09:13 PM: {Unknown} SendToAllInSight from TestAle02_name considering relative cell -1, -3(bitcount 0)
12/23/2016 10:09:13 PM: {Unknown} SendToAllInSight from TestAle02_name considering relative cell 0, -3(bitcount 0)
12/23/2016 10:09:13 PM: {Unknown} SendToAllInSight from TestAle02_name considering relative cell 1, -3(bitcount 0)
12/23/2016 10:09:13 PM: {Unknown} SendToAllInSight from TestAle02_name considering relative cell 2, -3(bitcount 0)
12/23/2016 10:09:13 PM: {Unknown} SendToAllInSight from TestAle02_name considering relative cell 3, -3(bitcount 0)
12/23/2016 10:09:13 PM: {Unknown} SendToAllInSight from TestAle02_name considering relative cell -3, -2(bitcount 0)
12/23/2016 10:09:13 PM: {Unknown} SendToAllInSight from TestAle02_name considering relative cell -2, -2(bitcount 0)
12/23/2016 10:09:13 PM: {Unknown} SendToAllInSight from TestAle02_name considers relative cell -2, -2 in bounds.
12/23/2016 10:09:13 PM: {Unknown} SendToAllInSight from TestAle02_name considering relative cell -1, -2(bitcount 1)
12/23/2016 10:09:13 PM: {Unknown} SendToAllInSight from TestAle02_name considers relative cell -1, -2 in bounds.
12/23/2016 10:09:13 PM: {Unknown} SendToAllInSight from TestAle02_name considering relative cell 0, -2(bitcount 2)
12/23/2016 10:09:13 PM: {Unknown} SendToAllInSight from TestAle02_name considers relative cell 0, -2 in bounds.

Another look at the server code, and we see why: bitcount is only incremented for cells that are in bounds. Since our test map is quite small, and players are spawning near the edge, a portion of their immediate area is out of bounds.

I could tweak the server code to behave the way I think it was intended to, but I feel certain that there are probably other edge-of-the-map issues waiting for me. So instead I’ll expand the test map so the players are always safely away from the edge.

maps/test01.txtlink
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

<z>0</z> <x>38</x> <y>23</y> <f>light</f> <n>Test01 Surface</n> outdoor=true
WWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWW
WWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWW
WWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWW
WWWWWW. . . . . . . . . . . WWWWWW
WWWWWW. . . . . . . . . a0. WWWWWW
WWWWWW. . . . . . . . . . . WWWWWW
WWWWWW. . . . . . . . . a1. WWWWWW
WWWWWW. . . . . . . . . . . WWWWWW
WWWWWW. . . . . . . . . gg. WWWWWW
WWWWWW. . . . . . . . . . . WWWWWW
WWWWWW. . . . . . . . . . . WWWWWW
WWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWW
WWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWW
WWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWW

12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name considering relative cell -1, 0(bitcount 23)
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name considers relative cell -1, 0 in bounds.
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name considers relative cell -1, 0 visible?
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name considers relative cell -1, 0 non-null?
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name considering relative cell 0, 0(bitcount 24)
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name considers relative cell 0, 0 in bounds.
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name considers relative cell 0, 0 visible?
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name considers relative cell 0, 0 non-null?
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name considering TestAle02_name.
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name considering TestAle02_name further.
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name sending "TestAle01_name burps loudly." to TestAle02_name.
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name sending (filtered) "TestAle01_name burps loudly." to TestAle02_name.
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name considering TestAle01_name.
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name considering relative cell 1, 0(bitcount 25)
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name considers relative cell 1, 0 in bounds.
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name considers relative cell 1, 0 visible?
12/24/2016 3:36:38 PM: {Unknown} SendToAllInSight from TestAle01_name considers relative cell 1, 0 non-null?

Much better.

124 scenarios (124 passed)
1241 steps (1241 passed)
50m36.933s

Interesting Stuff

Day 80 code - tests Day 80 code - server