Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: testify test suite support #58

Merged
merged 28 commits into from
Jul 11, 2024
Merged

Conversation

fredrikaverpil
Copy link
Owner

@fredrikaverpil fredrikaverpil commented Jun 25, 2024

image image

Please give me a hand and try it out:

  {
    "nvim-neotest/neotest",
    ft = { "go" },
    dependencies = {
      {
        "fredrikaverpil/neotest-golang",
        branch = "feat/test-suite-support",  -- ADD THIS
      },
    },
    opts = function(_, opts)
      opts.adapters = opts.adapters or {}
      opts.adapters["neotest-golang"] = {
        dap_go_enabled = true,
        testify = true,  -- ADD THIS
      }
    end,
  },

Aims to fix #25.

To do

  • Detect receiver method name using AST/tree-sitter.
  • Detect suite name using AST/tree-sitter.
  • Create lookup between AST-detected suite name vs receiver method name.
  • Add hacks all over the place, so to verify my overall, initial, idea.
  • Augument neotest position tree with lookup resolve for each position, prior to building runspec.
  • Generate lookup based on entire go project? (likely, yes)
  • Replace pos.ids before returning tree from ast.lua. Should be able to remove all added hacks.
  • DAP supported out of the box, piggybacking on existing code.
  • Verify proper output generated after test ran when running a suite.
  • Trigger lookup rebuild, but based on what... 🤔
  • Community feedback.
    • Added alternative syntax support in testify suite function.
    • Table tests support. Will likely not be part of initial implementation, just to realistically scope this down.
    • Fixed bug on confusing regular test functions with testify suite functions.
  • Cleanup refactoring
    • Refactor testify.lua into folder of files, to make it easier to become familiar with the structure/flow.
    • Add types to all functions and relevant dataclasses/tables.
    • Add comments/docstrings to each significant function.
    • Scrutinize treesitter query/lookup generation which mixes struct/receiver nomenclature, could be confusing.
    • Consider using public Neotest methods instead of accessing private members directly (not sure if any such public methods exists).
  • Tests. Likely want to have a tests/go/testify folder/package with individual test files that tests each scenario.
    • Expected lookup based on test file parsed.
    • Simple test using the official testify example from their docs.
    • One suite struct, several test files with test receiver methods.
    • Alternative suite function syntax.
    • Regular, trivial, test function should not be confused with suite function.
  • Settle on the naming of the option to opt-in to this behavior. Perhaps testify_enabled?
  • Write a separate docs section on this feature, as it comes with nuances, caveats and complex level of maintainability over time.

@fredrikaverpil fredrikaverpil self-assigned this Jun 25, 2024
@fredrikaverpil fredrikaverpil force-pushed the feat/test-suite-support branch 4 times, most recently from 26f071e to 5abb5e8 Compare June 25, 2024 21:14
@fredrikaverpil fredrikaverpil changed the title feat: test suite support feat(poc): test suite support Jul 2, 2024
@fredrikaverpil fredrikaverpil force-pushed the feat/test-suite-support branch 22 times, most recently from d601345 to 6f7caf9 Compare July 7, 2024 14:06
@fredrikaverpil fredrikaverpil force-pushed the feat/test-suite-support branch from 1994e36 to fcb12d4 Compare July 10, 2024 13:52
@fredrikaverpil
Copy link
Owner Author

fredrikaverpil commented Jul 10, 2024

@rcarriga if you are up for it, I would love your input on this PR from your perspective as the creator of Neotest. 😄

TL;DR - I am detecting namespaces, and in the context of Go testing these will be used as testify suites. Each suite can contain any number of Go tests. But I have to do some real shenanigans to pull it all off like modifying the Neotest tree.

There have been previous attempts to support testify suites in neotest-go (not this adapter 😅) from what I understand, but with significant caveats:

My approach looks like this:

  1. Using custom treesitter query, generate a lookup which correlates testify suite structs with Go tests that should run under a testify suite.
  2. Detect tests along with the Go receiver method struct as namespace (using lib.treesitter.parse_positions).
  3. Modify the Neotest tree, so to replace the name of the namespace into the proper suite name (using the lookup created in step 1) and merge duplicate namespaces.

I am especially interested in your input of what you think about how I access private Neotest members and modify the Neotest tree in tree_modification.lua. Maybe there are better and more robust ways to achieve this?
I'd also like to know if there's any way to create callbacks to certain Neotest events, so that the lookup map can be regenerated based on e.g. the user saving a *_test.go file.

@fredrikaverpil fredrikaverpil force-pushed the feat/test-suite-support branch 2 times, most recently from 906dbfd to 04828ee Compare July 10, 2024 23:26
@fredrikaverpil fredrikaverpil force-pushed the feat/test-suite-support branch 7 times, most recently from 84f0850 to f099f5e Compare July 11, 2024 17:06
@fredrikaverpil fredrikaverpil force-pushed the feat/test-suite-support branch from f099f5e to 92a0f4d Compare July 11, 2024 17:07
@fredrikaverpil fredrikaverpil force-pushed the feat/test-suite-support branch from 74b9348 to f44809b Compare July 11, 2024 17:27
@fredrikaverpil fredrikaverpil force-pushed the feat/test-suite-support branch from 9f7deb8 to c108e31 Compare July 11, 2024 17:45
@fredrikaverpil fredrikaverpil marked this pull request as ready for review July 11, 2024 17:46
@fredrikaverpil fredrikaverpil merged commit d723241 into main Jul 11, 2024
9 checks passed
@fredrikaverpil fredrikaverpil deleted the feat/test-suite-support branch July 11, 2024 17:48
@rcarriga
Copy link

Hey just catching up on this now, very nice!

I am especially interested in your input of what you think about how I access private Neotest members and modify the Neotest tree in tree_modification.lua. Maybe there are better and more robust ways to achieve this?

You could use lists directly instead of the tree. You can use tree:to_list() to convert to lists, make any changes needed, and then use Tree.from_list to parse it back again. I'm not sure if this is better than the current approach but at least you don't have to worry about maintaining relations such as the parent/children.

I'd also like to know if there's any way to create callbacks to certain Neotest events, so that the lookup map can be regenerated based on e.g. the user saving a *_test.go file.

Not currently though your adapter should be called anytime a user edits a test file to parse the file again, is that not sufficient?

@fredrikaverpil
Copy link
Owner Author

You could use lists directly instead of the tree. You can use tree:to_list() to convert to lists, make any changes needed, and then use Tree.from_list to parse it back again. I'm not sure if this is better than the current approach but at least you don't have to worry about maintaining relations such as the parent/children.

Sounds at least like one gain, so I'll make sure to try that out!

Not currently though your adapter should be called anytime a user edits a test file to parse the file again, is that not sufficient?

I've added an initial trigger to generate the lookup here. Where do you suggest I add this testify.lookup.generate() in, so to re-generate the lookup whenever the user edits a test file?

@rcarriga
Copy link

I've added an initial trigger to generate the lookup here. Where do you suggest I add this testify.lookup.generate() in, so to re-generate the lookup whenever the user edits a test file?

Ah OK so then if you just run that whenever discover_positions is called because that'll be called anytime a file is edited. One problem is however it's called per file so a large suite will trigger it many times on first startup. I would recommend adding some debouncing logic to it

@fredrikaverpil
Copy link
Owner Author

fredrikaverpil commented Jul 13, 2024

One problem is however it's called per file so a large suite will trigger it many times on first startup.

Yes, exactly, this is why I haven't implemented this re-generation of the lookup yet. If I just add it into discover_positions, it'll get triggered way too many times in a row. If your suggestion is to implement it there and look into debouncing, I'll do that 👍 but do you think it will be robust given that the user might have discovery.enabled = false...? 🤔 It needs to be triggered on filesave even if the summary window isn't open. EDIT: Yes, it seems to work just fine, actually, with discovery.enabled = false. Will try to debounce that and make it work then!

@rcarriga
Copy link

I think debouncing is perfectly fine for this, I implemented it for the summary window to render as it has the same issue of lots of calls being made to render. It's just a simple while true loop using nio event objects https://github.com/nvim-neotest/neotest/blob/32ff2ac21135a372a42b38ae131e531e64833bd3/lua/neotest/consumers/summary/summary.lua#L84-L86

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.

Support running single test in a testify suite
3 participants