chore(lint): add ast-grep lint rules and CI workflow

[flrgh]

Summary

This is an experiment in using ast-grep to craft our own semantic lint rules.

ast-grep is a tool for querying source code in a (relatively) language-agnostic manner. It allows us to write lint rules that target patterns that are specific to our codebase and therefore not covered by tools like luacheck.

In practice I expect this to be used mostly for lua files, but in theory we could use it to write lints for all sorts of languages/filetypes.

CI Components

Everything here runs in the new ast-grep.yml workflow, which is triggered on a PR event with changes to any ast-grep-related files (rules, config, etc) or any *.lua source code files.

Lint

Errors and warnings behave as expected and create annotations on the job summary when encountered:

screenshot

Self Test

ast-grep has a test feature which allows you to check your rules against code snippets. Additionally, the workflow runs some bash glue code that checks for well-formed rule files and, importantly, requires that all rules have tests.

New Lints

I've added 3 lints to start with:

• ngx-log-string-concat
  • files: kong/**
  • severity: error
  • matches: unsafe usage of ngx.log()
• assert-eventually-terminated
  • files: **/*_spec.lua
  • severity: error
  • matches: any unterminated assert.eventually() call site
• helpers-outside-of-setup
  • files: **/*_spec.lua
  • severity: warning
  • matches: using some spec.helpers functions outside of setup()

Some things could be warnings today but elevated to error level in the future.

KAG-6656

[flrgh]

The official action is a full-on JavaScript wrapper around npm install ast-grep && ast-grep scan [...]. With all the noise about github actions and needing to audit/lock things down, it makes more sense to just maintain these <10 lines of code ourselves.

[nowNick]

I love it 😍 !
🏆

[nowNick]

@flrgh - is it possible to run it locally with CLI? I see there's info about it on their docs page (link) but I'm not sure it'll pickup the GH actions folder to learn about the rules?

[flrgh]

@flrgh - is it possible to run it locally with CLI? I see there's info about it on their docs page (link) but I'm not sure it'll pickup the GH actions folder to learn about the rules?

Yup! The config file (sgconfig.yml) included in this PR configures all the paths and is located at the root of the repository tree, so running ast-grep from within /path/to/git/kong works:

~/git/kong/kong $ ast-grep scan --filter helpers-outside-of-setup spec/03-plugins/27-aws-lambda/99-access_spec.lua
warning[helpers-outside-of-setup]: Calling test setup helper function in the wrong scope
     ┌─ spec/03-plugins/27-aws-lambda/99-access_spec.lua:19:35
     │  
  16 │ ╭   describe("Plugin: AWS Lambda (access) [#" .. strategy .. "]", function()
     ·  
  19 │ │     local mock_http_server_port = helpers.get_available_port()
     │ │                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     · │
1441 │ │   end)
     │ ╰──────'
     │  
     = Avoid calling test setup functions outside of `setup()`/`lazy_setup()`.
       
       ## good
       ```lua
       describe("my test", function()
         local port_a
         local port_b
       
         lazy_setup(function()
           port_a = helpers.get_available_port()
         end)
       
         it("my test case", function()
           port_b = helpers.get_available_port()
         end)
       end)
       
       ## bad
       ```lua
       local port_a = helpers.get_available_port()
       describe("my test", function()
         local port_b = helpers.get_available_port()
       
         it("my test case", function()
         end)
       end)

[jschmid1]

really cool stuff 👍🏼

[tao12345666333]

I love ast-grep 🦍❤️⚡

[iykon]

great change!

Calling test setup helper function in the wrong scope
good to see complaints, could you help me understand this rule, why are these use case not good?

[locao]

This is really good stuff 🚀

[flrgh]

Calling test setup helper function in the wrong scope

could you help me understand this rule, why are these use case not good?

@iykon sure! The pattern that I want to catch and discourage is performing any test/fixture setup tasks at the module/file level or directly within a describe() block. Creating a temporary file is a pretty universal integration test pattern--let's use that as an example:

local function create_temp_file(context)
  print("creating temp file in " .. context .. " context")
  -- simulate slowness
  ngx.sleep(1)
end

local temp = create_temp_file("file")

describe("my test suite #suite", function()
  local other_temp = create_temp_file("describe()")

  it("my test case #testcase", function()
    -- do something with `temp`
  end)

  it("my second test case #testcase", function()
    -- do something with `other_temp`
  end)
end)

If we put this into a file and evaluate its behavior, some problems appear:

# excluding the entire suite still calls create_temp_file() once
$ time busted --exclude-tags suite demo_spec.lua
creating temp file in file context

0 successes / 0 failures / 0 errors / 0 pending : 0.975825 seconds

real    0m1.089s
user    0m0.052s
sys     0m0.035s

# excluding all test cases still calls create_temp_file() twice
$ time busted --exclude-tags testcase demo_spec.lua
creating temp file in file context
creating temp file in describe() context

0 successes / 0 failures / 0 errors / 0 pending : 1.974669 seconds

real    0m2.062s
user    0m0.049s
sys     0m0.037s

Even just listing test cases with no intent to run anything calls create_temp_file():

$ time busted --list demo_spec.lua
creating temp file in file context
creating temp file in describe() context
demo_spec.lua:11: my test suite #suite my test case #testcase
demo_spec.lua:15: my test suite #suite my second test case #testcase

real    0m2.060s
user    0m0.047s
sys     0m0.036s

Slow test execution due to eagerly performing unnecessary work is one problem. The bigger problem is that these kinds of setup functions often have side effects that live beyond the execution of the test suite (filesystem, database, etc):

1. at T0 run dbless tests w/ busted --exclude-tags postgres spec/02-integration
2. at T0 + N a test tagged as #postgres mutates $state even though we've excluded #postgres tests
3. at T0 + N + M a dbless test fails because it was expecting $state to be empty

Test execution order might not be deterministic, so this might not fail right away. Maybe most of the time the order of steps 2 and 3 are reversed, and everything works just fine. We have soooo much of this in our test code right now, and it's really painful to debug.