Close #424 - make tests run in coroutine to support asynchronous testing

[idanarye]

This has some slight changes of behavior, which I don't think matter:

1. Previously, running coroutine.running() inside a test would yield nil. Now it yield the thread. If anyone was checking it, it would be different now. Then again - I see no reason why anyone would check it.
2. Previously, running coroutine.yield() inside a test would fail. Now it yields. Again - I see no reason why anyone would do this before this PR which adds support to it.

I wanted to add an entry to CHANGELOG.md, but it seems unmaintained and irrelevant.

[Conni2461]

LGTM thanks :)

I didn't have an option to run CI, so i rebased it. I also fixed the stylua ci, so i can merge it

[Conni2461]

also it might be better to just drop the Changelog file, so no one is confused by it.

I'll gonna do that, so thanks for mentioning

[ipod825]

I am burned by this change.

https://github.com/ipod825/libp.nvim/search?q=coroutine.running

I actually check coroutine.running() to see if it's under plenary async context.
Do you know if there's a way to get that information from plenary library?
Otherwise, please consider reverting this change as I do think that being able to know if we are inside an async context is very useful.

[ipod825]

Basically what I want to do is to maintain a unified interface of some APIs such that they can be both called from main thread and a couroutine thread (mostly inside plenary coroutine). And the way of achieving that is to check coroutine.running() in the module __index and return different version of functions accordingly.

[ipod825]

Why not just use async.util.sleep() (inside a.it)

Is there any more complex use case that justify this change?

[idanarye]

async.util.sleep and a.it are doing more or less the same behind the scenes. The point of this minimal example was to demonstrate a resuming a coroutine from a callback - something which would usually include things like autocommands or event handlers on Neovim jobs. But these are a bit more complex to set up and activate, so to keep the example simple I used a simple defer_fn.

[Conni2461]

#424

It makes sense to have busted in a coroutine, at least sometimes. I currently think what we can do is have a global that you need to set to true, and then its enabled for this file otherwise its not running in a coroutine.

Or we do it over the filename: spec.lua vs async_spec.lua

Maybe i should reverts

[ipod825]

+1 for making this explicit & configurable.
Maybe something like require("plenary.async").tests.add_to_env()

Or we do it over the filename: spec.lua vs async_spec.lua

-1 for the idea, it would force me to put tests of the same module in separate files just for this convention.

[Conni2461]

i suggested filename because i thought its a pain, to solve this inside a file, but it seems to work: #447

Just put in RUN_INSIDE_COROUTINE = true and the busted tests for that file run inside a coroutine. On default they don't. Does that work for you?

[idanarye]

Another option is to have all the tests that need async run inside a describe_asnyc (name to be bikesheded) instead of describe. This will register these tests in a list, and after running all the regular tests, plenary will start a coroutine that runs all the tests from that list.

[idanarye]

BTW, @ipod825, isn't your check kind of dangerous? What if I do something like this?

local fsync = require("libp.fs.uv_async").fs_fsync

coroutine.create(function()
    fsync(some_path)
end)

[ipod825]

Just put in RUN_INSIDE_COROUTINE = true and the busted tests for that file run inside a coroutine. On default they don't. Does that work for you?

SGTM

BTW, @ipod825, isn't your check kind of dangerous? What if I do something like this?

Yeah. That's a caveat. The APIs are designed to be unified in main thread and plenary coroutine. It probably doesn't work well in vanilla coroutine. Unless plenary provides a function that tells if we are in a plenary coroutine, I can't distinguish the two.

[idanarye]

How would RUN_INSIDE_COROUTINE = true even work, technically? describe executes the function passed to it, rather than registering it to be executed later, so by the time the harness can read the global RUN_INSIDE_COROUTINE - that function has already been executed.

Unless you are suggesting that describe reads the global RUN_INSIDE_COROUTINE and if true acts like the describe_asnyc I've suggested?

The APIs are designed to be unified in main thread and plenary coroutine. It probably doesn't work well in vanilla coroutine. Unless plenary provides a function that tells if we are in a plenary coroutine, I can't distinguish the two.

Shouldn't you want something like that regardless of this PR? Other things can create coroutines too.

Also - if your problems are caused by my PR not using a plenary coroutine, wouldn't a simple fix be to just have it use plenary coroutines?

[ipod825]

Unless you are suggesting that describe reads the global RUN_INSIDE_COROUTINE and if true acts like the describe_asnyc I've suggested?

I think that is how #447 was implemented. Although I didn't see it reset to false somewhere. Meaning that once it's set, all other test are forced to run in a coroutine.

Shouldn't you want something like that regardless of this PR? Other things can create coroutines too.

Yes. But managing coroutine manually is really painful and cumbersome. I don't see any chance that I would choose to use vanilla coroutine but plenary coroutine. So I only care about plenary coroutine compatibility. I don't care if it doesn't work in vanilla coroutine. If someone want's to use those APIs, I'll say, either use plenary coroutine (which is easier) or don't use those APIs. And I am not knowledgeable enough on how I can define a function to see if we are inside a vanilla coroutine or plenary coroutine.

Also - if your problems are caused by my PR not using a plenary coroutine, wouldn't a simple fix be to just have it use plenary coroutines?

No, consider the following test

describe("__index", function()
    it("Gets vim.loop function in main thread", function()
        assert.are.equal(vim.loop.fs_open, uv.fs_open)
    end)
end)

uv.fs_open should return vim.loop.fs_open in main thread and an asyn version in a coroutine. Regardless of whether it's plenary coroutine or vanilla coroutine, the test always fail if run inside a coroutine.

[Conni2461]

How would RUN_INSIDE_COROUTINE = true even work, technically?

I implemented it before you suggested describe_async, but yeah thats basically how its done. #447

Although I didn't see it reset to false somewhere

You enable it per file and test files are isolated so we dont have to reset anything

descibe_async is also not that bad of an idea but i then we have to think about this:

describe_async("", function()
  describe("", function()
    -- how does this work?!
  end)
end)

and this

describe("", function()
  describe_async("", function()
    -- how does this work?
  end)
end)

This makes everything a bit confusing, so filebased toggle might be enough, especially because we already have

require("plenary.async").tests.add_to_env()

a.describe
...