-
-
Notifications
You must be signed in to change notification settings - Fork 283
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
Close #424 - make tests run in coroutine to support asynchronous testing #426
Conversation
43d2b65
to
4c3c5f2
Compare
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 |
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 |
I am burned by this change. https://github.com/ipod825/libp.nvim/search?q=coroutine.running I actually check |
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.resume(co) | ||
end, 1000) | ||
--The test will reach here immediately. | ||
coroutine.yield() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use async.util.sleep() (inside a.it)
Is there any more complex use case that justify this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
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: Maybe i should reverts |
+1 for making this explicit & configurable.
-1 for the idea, it would force me to put tests of the same module in separate files just for this convention. |
i suggested filename because i thought its a pain, to solve this inside a file, but it seems to work: #447 Just put in |
Another option is to have all the tests that need async run inside a |
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) |
SGTM
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. |
How would Unless you are suggesting that
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? |
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.
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.
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)
|
I implemented it before you suggested
You enable it per file and test files are isolated so we dont have to reset anything
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
... |
…ting (nvim-lua#426)" This reverts commit 1252cb3.
This has some slight changes of behavior, which I don't think matter:
coroutine.running()
inside a test would yieldnil
. 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.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.