Unit Test Bootstrapping

[keithharvey]

What this does

Adds a set of configuration defaults, CI, and mocks/builders for the busted unit testing framework with an example spec. Updated the readme with installation instructions and examples.

Related Work

• (REQUIRED / these should go together) Lux Package Manager

Docs

• Busted Github The unit test library itself.
• Busted Docs Really useful
• Luassert Github The assert syntax library busted uses under the covers
• Luassert Community Type Library Github copied into types/luassert/, with attribution

Why Unit Testing

You get to make assertions on idempotent code and see what pops out the other side in <1s. Very useful for breaking down moderate to complex problems.

Abstracts Global State

We try not to touch global state in unit tested code because it makes tests fragile and verbose. Instead, we pass in our dependencies (like Spring, or in the case of a single function its parameters).

Builder (Mocks)

I think the builders are the part of this PR that deserves the most scrutiny since that is one of the more opinionated parts of this test setup and their design will be super important to having elegant tests.

The builder pattern is my default for building mocks when unit testing across languages because it provides good intellisense and navigate-to-definition support and is relatively straightforward. You can create the builder in a parent closure/describe block, then leave it a builder until you are ready to test some specific condition, and then call builder:Build() to create the mock. An example:

describe("my doodad", function()
    local springBuilder = Builders.Spring.new()
    ---@type Doodad
    local doodad
 
    describe("my sub-behavior", function() {
        -- note that things you want to happen before EVERY test in this closure should be in the `before_each` block
        springBuilder:WithModOption(ModOptions.doodad, "enabled")

        local spring
        before_each(function()
            local spring = springBuilder:Build()
            doodad = Doodad.new(spring)
        end)
        it("should do something very specific when I call my function", function()
           expectedResult = 42
           result = doodad.DoSomethingCool()
           assert.are.equal(expectedResult, result)
        end)
    end)
    
    describe("my other behavior", function()
        springBuilder:WithModOption(ModOptions.doodad, "disabled")
        ....
    end)
end)

Mocks

spring_builder

Builds a mock for implementations that consume ISpring to use instead. This builder provides a number of helper functions such as WithAlliance, AddTeamResources, and mod option support.

It also provides a WithGlobalsDefined(fun: void), which allows easy loading of files that depend on global state into a local spec closure.

local unitDefs
SpringBuilder:WithGlobalsDefined(function()
   unitDefs = doGlobalStuff()
end)

teamBuilder

Provides an interface to set up a specific team's properties

For example:

local me = Builders.Team.new()
   :Human()
   :WithMetal(100)
   :WithUnitFromCategory(BuildingCategories.PINPOINTER, Sides.ARMADA)
local myAlly = Builders.Team.new()
    :AI()
    :WithMetal(1000)
-- it can then be passed into the spring builder directly
springBuilder
   :WithTeam(me)
   :WithTeam(myAlly)
   :WithAlliance(me, myAlly, true)
-- the team values will now be correctly wired up to the correct engine calls in the mock
local spring = springBuilder:Build()

I tend to not like static mock libraries very much (even in strongly typed languages like C#, I'm thinking of Moq) and prefer shared, bespoke builders representing things in my domain versus each test creating its own mocks derived from type definitions. I doubt this would be super viable in a dynamic language like LUA-5.1, but thought it worth mentioning as an alternative pattern I have seen.

Asks

• Does spring publish publish an EmmyLua ISpringGadgets and ISpringWidgets somewhere? This would be helpful for mocks to conform to. Do we want to add these types/interfaces to the engine side generation? Is that the right names for them?
  • ISpringGadgets -> recoil-lua-library/library/generated/rts/Lua/LuaSyncedCtrl.cpp.lua
  • ISpringWidgets -> recoil-lua-library/library/generated/rts/Lua/LuaUnsyncedCtrl.cpp.lua
• Do we like the initial implementation of SpringBuilder with its builder pattern and fluent With interface?
• Do we like passing individual sub-builders into parent builders, for example springBuilder:WithTeam(teamBuilder)? Would we prefer SpringBuilder just handled the setup for its own api entirely by itself without the builder composition?

LLM DIsclosure

I am sure some of these builders were touched by an LLM at some point but I've been working this code for a while myself now.

[NortySpock]

(senior developers, please let me or keithharvey know if there was some other unit testing framework already in use that we missed.)

@keithharvey I have questions about where the type-check code came from (see the review comment later down on "how is this licenced, exactly?"), but I would love to be able to have these non-engine-based unit tests so we can write tests quickly and not manually need to check everything.

(for those unfamiliar with unit tests, the advantage is you can write code that tests one or two functions together, easily, and those tests help monitor for accidental changes or unexpected edge cases.)

To install the unit test framework, a Debian Linux developer would run

sudo apt install luarocks
sudo luarocks install busted

and then could list all the tests with

busted -l

spec/gamedata/unitdefs_spec.lua:10: UnitDefs should be loaded
spec/gamedata/unitdefs_spec.lua:22: UnitDefs should have valid structure

and then run all the tests with

busted

●●
2 successes / 0 failures / 0 errors / 0 pending : 0.265552 seconds

If we can add tests to verify that a function or module "does what it says on the tin" when looking for a bug or when making a change, we can easily test or disprove hypotheses. We can also support safe refactoring of code that we wanted to clean up, by wrapping it in tests that characterize the current behavior, splitting it out into smaller modules (that likely also get tests) and verify that the system has the same tests pass before and after the code change. It allows you to make changes with less need for manual testing.

[keithharvey]

Edited the description to better highlight the parts of this PR better that need scrutiny. There's some examples of more complicated specs in another branch I am working on if anyone is curious what that would look like.

[keithharvey]

It would be nice if recoil-lua-library/library/generated/rts/Lua/LuaUnsyncedCtrl.cpp.lua and SyncedCtrl would publish an equivalent EmmyLua class type to Spring's API (ISpring?) we could apply to the mock.

Edit: I deleted spring_repository, it wasn't a necessary concept when we can just pass Spring in from the gadget. I am out of this branch again now

[przystuj]

I'm all for unit tests but copying the code of the other library sounds off to me. Is there no better way? What if they update their library with critical fixes, do we have to remember to manually copy paste it again? Maybe I'm spoiled with the java world but it just feels wrong to me.

[NortySpock]

I'm all for unit tests but copying the code of the other library sounds off to me. Is there no better way? What if they update their library with critical fixes, do we have to remember to manually copy paste it again? Maybe I'm spoiled with the java world but it just feels wrong to me.

I get that, but I'm in favor of merging this as-is so we get something working. Figuring out how to write the config file for the luarocks package manager sounds like extra steps that, while possibly useful in the future, don't solve the problem now of "we just need code that runs unit tests to allow people to write unit tests."

"Make it work, then make it right, then make it fast" - Kent Beck

We're still at "Make it work"

[efrec]

I'm all for unit tests but copying the code of the other library sounds off to me. Is there no better way? What if they update their library with critical fixes, do we have to remember to manually copy paste it again? Maybe I'm spoiled with the java world but it just feels wrong to me.

I get that, but I'm in favor of merging this as-is so we get something working. Figuring out how to write the config file for the luarocks package manager sounds like extra steps that, while possibly useful in the future, don't solve the problem now of "we just need code that runs unit tests to allow people to write unit tests."

"Make it work, then make it right, then make it fast" - Kent Beck

We're still at "Make it work"

Have to agree that this puts me off the PR.

[keithharvey]

I'm all for unit tests but copying the code of the other library sounds off to me. Is there no better way? What if they update their library with critical fixes, do we have to remember to manually copy paste it again? Maybe I'm spoiled with the java world but it just feels wrong to me.

It absolutely is wrong. It also feels off to me. Unfortunately in this case I think we don't have a good package manager setup in the project so the alternatives are submodules, which imo are worse. These are strictly type definitions and I did include a provenance.md so in a few years if we notice the types don't match, someone can go grab the /types folder out of busted. I suspect these are mostly going to be static. If someone wants to make the decision on a package manager for this project, I would love to go set that up. But I didn't want to roll that boulder up the hill for this alone.

[keithharvey]

Well shoot I just noticed I did a bad job pulling work out of my sharing tab branch, moving this back to draft for a second while I clean up the mocks.

[keithharvey]

• cleaned this up to be more compatible with my work over in Recoil refactor(Lua): Spring.X -> SpringBucket.X RecoilEngine#2799, basically I just named it "SpringSynced" instead of "ISpring" for my mock type inheritance.
• improved the Readme with better, more accurate examples
• locked the CI to lua 5.1
• it drives me INSANE that the Integration Test Result publisher is both mislabelled and grouped under the wrong workflow. This is a byproduct of it being asynchronous, the order being dictated by alphabetical order

I did rename the files so the alphabetization should help and fix the check_name on the post-processing job to be to be "Integration Test Results" but we won't see that until this hits master.