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

fix: pending tests, single/double quotes, logger #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

YaroSpace
Copy link

This is just to show the changes I have made locally. Still investigating the json encoding of non-string objects.


if ok then
file:write(result)
file:close()
else
io_write("Failed to encode test results to json: " .. result .. "\n")
io_flush()
os.exit(1)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? Is it because the process cannot continue in a sane way if json encoding failed?

Copy link
Author

@YaroSpace YaroSpace Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that and also busted is often run as a separate process wrapped in status, error = pcall(..) for example. Setting the exit code, allows to assert the error. Neotest also runs the tree with the spec that your adapter provides wrapped in xpcall and asserts the exit status.

I have actually updated the busted json formatter here https://github.com/lunarmodules/busted/pull/748/files

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see but couldn't you assert on the error if you returned false as the second return value? Or called error? I don’t believe os.exit is necessary for pcall/xpcall to register an error.

Nice that you contributed to busted (and that it seems maintained). We can wait and see what the outcome of that PR is since it would probably be a good idea to align with what busted’s builtin handlers do.

Comment on lines +382 to +387
;; pending blocks
((function_call
name: (identifier) @func_name
arguments: (arguments (_) @test.name (function_definition))
) (#match? @func_name "^pending$")) @test.definition

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve had a chance to play around with this query and there are a few corner cases.

  1. In the example you gave here Help with busted, local-lua config 🙏  #17 (comment), the call to pending in the first test should not be matched, only the it test itself should. I think it needs match calls to pending whose parent is a describe. Running busted —list <spec> on the file shows what busted considers a test.
  2. The query won’t match calls to pending without a second function argument.

I’ve got some working queries locally for these cases and I’ll continue to test it out a bit to make sure I haven’t missed something.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree. That was a quick copy/paste from it block.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, I figured it was. Here’s what I got so far. Haven’t found a better way yet to encode the parent-child relationship.

;; pending blocks constrained to have a describe as a parent
((function_call
    name: (identifier) @func_name1 (#match? @func_name1 "^describe$")
    arguments: (arguments
        (_) @namespace.name
        (function_definition
        parameters: (parameters)
        body: (block
            ((function_call
                name: (identifier) @func_name2
                arguments: (arguments (_) @test.name (function_definition)?)
            ) (#match? @func_name2 "^pending$")
            ) @test.definition
        )
        )
    )
)) @namespace.definition

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid this does not work properly with Neotest summary panel.
image
it('this test does not run..) should not come up in the panel, as it is inside the pending block'

I would also argue, that top level pending should be parsed as it does come up with busted --list and it is quite convenient to mark a whole buch of tests to pending by just renaming describe to pending.

Copy link
Author

@YaroSpace YaroSpace Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    ;; pending blocks
    ((function_call
        name: (identifier) @func_name
        arguments: (arguments (string) @test.name)
    ) (#match? @func_name "^pending$")) @test.definition

this fixes it.
I would not bother trying to encode the nested cases:

  • in the case pending..it - all nested it are skipped anyway
  • in the case of it..pending, busted correctly marks the parent it as pending

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this. I've been sick for the past 2 weeks and hadn't made much headway.

it('this test does not run..) should not come up in the panel, as it is inside the pending block'

I can reproduce it. Not sure why exactly this happens.

I would also argue, that top level pending should be parsed as it does come up with busted --list and it is quite convenient to mark a whole buch of tests to pending by just renaming describe to pending.

I agree. Constraining the pending to be inside a describe was a bit too restrictive.

I don't think we should constrain the argument to be a string since that would break parametric tests such as it("a" .. i). The original query accounts for this.

  • in the case pending..it - all nested it are skipped anyway

In this case, the test fails and the output is something like 0 successes / 0 failures / 0 errors / 0 pending : 0.000567 seconds since neotest-busted doesn't get any json output for the test. This may be acceptable for now.

  • in the case of it..pending, busted correctly marks the parent it as pending

I get the correct pending output for this case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to hear about your illness. I hope you are better now!

Ah, yes..forgot about parametric tests. You are right!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 🙂 it was just a bad case of the flu and I'm much better now.

If you have time you are welcome to try out #41 which I think can be merged as a good foundation for supporting pending tests.

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.

2 participants