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

php: Add runnable tests #11514

Merged
merged 20 commits into from May 22, 2024
Merged

Conversation

RemcoSmitsDev
Copy link
Contributor

@RemcoSmitsDev RemcoSmitsDev commented May 7, 2024

This pull request adds the following:

  • Missing mapping for the yield keyword.
  • Outline scheme for describe, it and test function_call_expressions (to support Pest runnable)
  • Pest runnable support
  • PHPUnit runnable support
  • Task for running selected PHP code.

Queries explanations

Query 1 (PHPUnit: Run specific method test):

  1. Class is not abstract (because you cannot run tests from an abstract class)
  2. Class has Test suffix
  3. Method has public modifier(or no modifiers, default is public)
  4. Method has test prefix

Query 2 (PHPUnit: Run specific method test with @test annotation):

  1. Class is not abstract (because you cannot run tests from an abstract class)
  2. Class has Test suffix
  3. Method has public modifier(or no modifiers, default is public)
  4. Method has @test annotation

Query 3 (PHPUnit: Run specific method test with #[Test] attribute):

  1. Class is not abstract (because you cannot run tests from an abstract class)
  2. Class has Test suffix
  3. Method has public modifier(or no modifiers, default is public)
  4. Method has #[Test] attribute

Query 4 (PHPUnit: Run all tests inside the class):

  1. Class is not abstract (because you cannot run tests from an abstract class)
  2. Class has Test suffix

Query 5 (Pest: Run function test)

  1. Function expression has one of the following names: describe, it or test
  2. Function expression first argument is a string

PHPUnit: Example for valid test class

Screenshot 2024-05-08 at 10 41 34

PHPUnit: Example for invalid test class

All the methods should be ignored because you cannot run tests on an abstract class.
Screenshot 2024-05-07 at 22 28 57

Pest: Example

Screen.Recording.2024-05-08.at.21.29.51.mov

You should now see all your Pest tests inside the buffer symbols modal.
Screenshot 2024-05-08 at 22 51 25

Release Notes:

  • Added test runnable detection for PHP (PHPUnit & Pest).
  • Added task for running selected PHP code.
  • Added describe, test and it functions to buffer symbols, to support Pest runnable.
  • Added yield keyword to PHP keyword mapping.

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label May 7, 2024
Copy link
Contributor

@claytonrcarter claytonrcarter left a comment

Choose a reason for hiding this comment

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

Cool! Just leaving a few comments based on a version that I was working on before I saw that someone had beat me to it! 😄

extensions/php/languages/php/runnables.scm Outdated Show resolved Hide resolved
extensions/php/languages/php/tasks.json Outdated Show resolved Hide resolved
@RemcoSmitsDev
Copy link
Contributor Author

Cool! Just leaving a few comments based on a version that I was working on before I saw that someone had beat me to it! 😄

Ahhh sorry man, didn't know that. I hope this satisfies your needs!

Copy link
Contributor

@osiewicz osiewicz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Remco :)

@RemcoSmitsDev
Copy link
Contributor Author

LGTM, thanks Remco :)

I gonna add pest & docblock annotation support aswell. Working on it right now!

@RemcoSmitsDev RemcoSmitsDev marked this pull request as draft May 8, 2024 07:59
Error: multiple different run targets found on a single line, only the last target will be rendered
@RemcoSmitsDev RemcoSmitsDev marked this pull request as ready for review May 8, 2024 08:35
@claytonrcarter
Copy link
Contributor

Ahhh sorry man, didn't know that. I hope this satisfies your needs!

It definitely does. No worries here! Thank you!

@RemcoSmitsDev
Copy link
Contributor Author

Ahhh sorry man, didn't know that. I hope this satisfies your needs!

It definitely does. No worries here! Thank you!

You want to hack on the pest support together?

Copy link
Contributor

@claytonrcarter claytonrcarter left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you to doing this. It's certainly more robust that what I had hacked together! 😆

@RemcoSmitsDev
Copy link
Contributor Author

@osiewicz Can you help me out, I have the following 2 queries. They seem to work because I can see the runnable icon before the test functions. But when I click on them nothing happens, what am I missing here? Or not understanding correctly? It seems like it cannot get the value correctly from the query.

Screenshot 2024-05-08 at 16 06 07

This query support non-chaining methods:

test("example", function () {
    expect(true)->toBeTrue();
});
(
    (expression_statement
        (function_call_expression
            function: (_) @name
            (#any-of? @name "it" "test" "describe")
            arguments: (arguments
                (argument
                    (encapsed_string (string_value)) @run
                )
            )
        )
    )
) @pest-test

This query support chaining methods:

test("example", function () {
    expect(true)->toBeTrue();
})->with(["1", "2"]);
(
    (expression_statement
        (member_call_expression
            object: (function_call_expression
                function: (_) @name
                (#any-of? @name "it" "test" "describe")
                arguments: (arguments
                    (argument
                        (encapsed_string (string_value)) @run
                    )
                )
            )
        )
    )
) @pest-test

…ne scheme

This will allow to run the Pest runnable. Because before this change
These function call expressions were not recognized as symbols.

This resulted in that the Symbol key inside the task context not being set.
@RemcoSmitsDev
Copy link
Contributor Author

RemcoSmitsDev commented May 8, 2024

While digging inside the runnable/task code, looking why clicking the runnable icon inside the gutter for Pest runnable didn't open a new terminal panel to run the tests in. I finally found the issue that not only applies to Pest runnable.

The issue is that the following code does not contain any symbols according to the Zed PHP outline scheme.

<?php

test("example", function () {
    expect(true)->toBeTrue();
});

describe("tests", function () {
    test("test1", function () {
        expect(true)->toBeTrue();
    });

    test("test2", function () {
        expect(true)->toBeTrue();
    });
});

it("has a name")
    ->expect(fn() => User::create(["name" => "Nuno Maduro"])->name)
    ->toBe("Nuno Maduro");

So when the task context is getting build inside task_context::build_context (line: 53). We ask for all the symbols, but this will give us an empty Vec back. Because we have an empty Vec we cannot determine what the symbol for the task context should be.

So if we go higher up the stack we are trying to build a task from the task template, which could not be done because the symbol information does not exist inside the context which is required for the Pest task template.


So I was talking to @osiewicz to explain what I found, why the Pest runnable did not work. So I came up with a fix for this issue by adding the following code to the PHP outline scheme:

; Add support for Pest runnable
(function_call_expression
    function: (_) @context
    (#any-of? @context "it" "test" "describe")
    arguments: (arguments
        (argument
            (encapsed_string (string_value) @name)
        )
    )
) @item

After adding this to the outline scheme, I got the Pest runnable too work😀 That being said, as I already mentioned we are going to have this issue with more languages. So @osiewicz came up with an idea to store the query captures inside the Env key of the task context. So we could use them instead of having to add these expressions to the outline scheme.

Screenshot 2024-05-08 at 22 51 25


Note

We have to think about how we want to solve this in the feature. For now, having them inside the outline scheme is fine.

This allows people to use the following code:

```php
it("has a name")
    ->expect(fn() => User::create(["name" => "Nuno Maduro"])->name)
    ->toBe("Nuno Maduro");

--- And ---

describe("tests in describe", function () {
    test("test1", function () {
        expect(true)->toBeTrue();
    });

    test("test2", function () {
        expect(true)->toBeTrue();
    });
})->with(["1", "2"])->group("group1");
```
This allows multiple chainable methods to be run in a single test
We have to do this because we cannot specify dynamic child node matching for more than one node.

If you would have more than one chained method the previous version did not work.

NOTE: This will also match test("asdf") inside function/method declarations..
@RemcoSmitsDev
Copy link
Contributor Author

RemcoSmitsDev commented May 9, 2024

@osiewicz one thing I'm not happy about yet is that the Pest query/outline scheme is not specific enough. This results in invalid results that should not happen, but could.

Screenshot 2024-05-09 at 11 45 33

The original query/scheme that I made prevented this, but did not support more than one chained method.
This is because the more chained methods you have, the more member_call_expressions you have as child nodes. I could not fixture out how I could write a query/scheme that has dynamic wildcard for more than one child node using the (_) syntax.

So ideally I want to wildcard all the member_call_expressions that are inside the expression_statement. Is there away to do this?

I had a query similar to this to wildcard a single child node.

(
    (expression_statement
        (_
            (function_call_expression
                function: (_) @name
                (#any-of? @name "it" "test" "describe")
                arguments: (arguments
                    (argument
                        (encapsed_string (string_value)) @run
                    )
                )
            )
        )
    )
) @pest-test
Screenshot 2024-05-09 at 11 43 43

@osiewicz
Copy link
Contributor

osiewicz commented May 9, 2024

Hey, wanna pair on it sometime later in the day? E.g. at 5 PM (we're in the same timezone).

@RemcoSmitsDev
Copy link
Contributor Author

Yeah sure, sounds good!

@claytonrcarter
Copy link
Contributor

This results in invalid results that should not happen, but could.

Can Pest tests be defined in classes, or are they only correct when defined at the "top level" of the file/script? If so, could it work to query for any function_call_expression that are not in a class_declaration?

@RemcoSmitsDev
Copy link
Contributor Author

RemcoSmitsDev commented May 9, 2024

Pest does not support the following methods it, test and describe inside classes or function declarations. Because these functions are expressions. That way they are register as a test case. You if you would define them in any declaration kind it's not getting registered as a test case.

Yeah, we have to do something like that. But I'm not sure how to do that.

@claytonrcarter
Copy link
Contributor

Pest does not support the following methods it, test and describe inside classes or function declarations

If this is the case, would it work to wrap the query in (program (expression_statement ...)) to only match expressions at the "top level" and not, like in your example, within a class method? Hmm, though I guess that wouldn't work if the tests were wrapped in if (...) { } or foreach (...) { }?

Is there a tree-sitter way to query for something that does not have a particular node as an ancestor?

@RemcoSmitsDev
Copy link
Contributor Author

RemcoSmitsDev commented May 9, 2024

Yeah, adding the program as top level node inside the query fixes the issue that it matches also expressions inside the function/class declarations. The query looks like this:

; Match non chained methods for Pest test
(program
    (expression_statement
        (function_call_expression
            function: (_) @_name
            (#any-of? @_name "it" "test" "describe")
            arguments: (arguments
                .
                (argument
                    (encapsed_string (string_value) @run)
                )
            )
        )
    )
) @pest-test

So I expect that no one wraps them inside an if or foreach state. Because then you are using Pest totally in the wrong way. So let's not worry about that.

@mansoorkhan96
Copy link

@RemcoSmitsDev possible to add support for Laravel Dusk?

@RemcoSmitsDev
Copy link
Contributor Author

@mansoorkhan96 You can make that possible by your self, when this pull request is merged by adding the following task to your ~/.config/zed/tasks.json

{
  "label": "laravel dusk test $ZED_SYMBOL",
  "command": "php artisan dusk",
  "args": ["--filter $ZED_SYMBOL $ZED_FILE"],
  "tags": ["phpunit-test", "pest-test"]
}

@osiewicz
Copy link
Contributor

@RemcoSmitsDev I've merged main into this branch btw

@osiewicz osiewicz merged commit bfdd9d8 into zed-industries:main May 22, 2024
10 checks passed
@osiewicz
Copy link
Contributor

Thanks!

@RemcoSmitsDev RemcoSmitsDev deleted the php-test-detection branch May 22, 2024 12:00
@maxdeviant maxdeviant mentioned this pull request May 22, 2024
maxdeviant pushed a commit that referenced this pull request May 22, 2024
This PR bumps the PHP extension to v0.0.4.

Changes:

- #11514

Release Notes:

- N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants