Skip to content

Conversation

@peterfox
Copy link

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes

Adds an extension called TestCaseMakesHttpRequestsExtension and renames TestCaseExtension to TestCaseMockeryExtension.

The purpose of TestCaseMakesHttpRequestsExtension is to apply the return type of Illuminate\Testing\TestResponse to HTTP request testing methods such as get, getJson and post etc.

At current, these methods will return a mixed type meaning PHPStan can't process tests using these methods effectively.

I'm new to making changes to PHPStan codebases and rules so if there's a better way of doing this or something I've missed in the PR, please feel free to guide me in the best way of handling this.

Breaking changes

None

@peterfox peterfox changed the title Adds extension for MakesHttpRequests feat: extension for MakesHttpRequests Dec 13, 2022
@canvural
Copy link
Member

Hi,

Thanks for the PR!

But I do not understand why is this needed. If I look at the types for the get method for example, it looks fine to me: https://github.com/laravel/framework/blob/9.x/src/Illuminate/Foundation/Testing/Concerns/MakesHttpRequests.php#L300-L307

@peterfox
Copy link
Author

peterfox commented Dec 13, 2022

I also believed this to be the case, but it is not. I built a demo repo you can try out https://github.com/peterfox/rector-status-rule-demo. If you use it with Larastan or not, it won't work because, for some reason, the return type found by PHPStan on those methods is mixed and not TestResponse.

I'm not sure if it's because it's a trait or something like that, but the only way I could make PHPStan give the correct return type was by adding the method as a doctag to the TestCase above the class.

/*
 * @method TestResponse get(string $path, array $headers)
 */

@canvural
Copy link
Member

I just cloned your example repository, and made the following changes:

diff --git a/phpstan.neon b/phpstan.neon
index f04455f..4578715 100644
--- a/phpstan.neon
+++ b/phpstan.neon
@@ -5,6 +5,7 @@ parameters:
 
     paths:
         - app/
+        - tests
 
     # Level 9 is the highest level
     level: 5
diff --git a/tests/Feature/ExampleTest.php b/tests/Feature/ExampleTest.php
index 1eafba6..b377872 100644
--- a/tests/Feature/ExampleTest.php
+++ b/tests/Feature/ExampleTest.php
@@ -16,6 +16,8 @@ public function test_the_application_returns_a_successful_response()
     {
         $response = $this->get('/');
 
+	\PHPStan\dumpType($response);
+
         $response->assertStatus(200);
     }
 }

Then when I run ./vendor/bin/phpstan the result is:

------ ---------------------------------------------- 
  Line   tests/Feature/ExampleTest.php                 
 ------ ---------------------------------------------- 
  19     Dumped type: Illuminate\Testing\TestResponse  
 ------ ----------------------------------------------

Also, tested the same thing in another project. Same result. I can't produce getting mixed as the dumped type 🤷🏽‍♂️

@canvural
Copy link
Member

Also if I run Rector on your repo:

1 file with changes
===================

1) tests/Feature/ExampleTest.php:17

    ---------- begin diff ----------
@@ @@

 	\PHPStan\dumpType($response);

-        $response->assertStatus(200);
+        $response->assertOk();
     }
 }
    ----------- end diff -----------

Applied rules:
 * AssertStatusToAssertMethodRector

so looks like it works correctly.

@peterfox
Copy link
Author

@canvural thank you for showing me that technique to perform a debug. I can confirm that you're right PHPStan is dumping the correct type as you've demonstrated with your code.

It looks rector is still failing for me which is a bit of a head scratcher.

Would you be able to just provide me what versions you're using for Larastan, PHPStan, Rector and PHP if possible?

@canvural
Copy link
Member

I just did composer install on your repo. So the latest version of everything.

  • Larastan 2.2.9 and your dev-feature/testcase-return-types branch
  • PHPStan 1.9.3
  • Rector 0.15.0
  • PHP 8.2

But I also realized your repo uses your branch for the Larastan. I changed it to 2.2.9, still the same result though.

@peterfox
Copy link
Author

Thank you, it does appear we're on the same versions, I wasn't sure if maybe it was a PHP version missmatch.

To check, you might want to run ./vendor/bin/rector --clear-cache as I find it can often cache things in a way you don't realise is affecting stuff after the change of the Larastan version.

Thank you for helping with this by the way. I greatly appreciate this goes beyond a maintainer's normal workflow.

@canvural
Copy link
Member

Yeah when I ran ./vendor/bin/rector --clear-cache it didn't change the file when Larastan 2.2.9 is installed. But PHPStan still dumps the correct type 🤷🏽‍♂️

@peterfox
Copy link
Author

😆 well, I'm glad it's not just me then, though I hoped it was just something dumb I'd done. This might be a bug in rector somehow.

The next step is to find out if that's the case, I guess. Are you okay with this PR staying open in the meantime?

@canvural
Copy link
Member

Honestly I don't think this PR will be the solution. The extension here kinda doesn't make sense.

If you really find an issue later, you can open a new one. Hope you understand.

PS. I'll also take a look at this issue, because it's interesting.

@peterfox
Copy link
Author

@canvural totally understand 👍 . Please let me know if you find something or if I can help. Thank you for taking the time to sanity-check my PR at least.

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