phpunit icon indicating copy to clipboard operation
phpunit copied to clipboard

(doc) TestCase::$backupStaticAttributes can be NULL.

Open donquixote opened this issue 4 years ago • 3 comments

I found this minor issue with psalm on a custom project that uses phpunit.

psalm: PropertyNotSetInConstructor: Property Donquixote\QuickAttributes\Tests\ClassesTest::$backupStaticAttributes is not defined in constructor of Donquixote\QuickAttributes\Tests\ClassesTest or in any methods called in the constructor

Psalm is correct. Either the property type must allow null, or the property must be initialized in the constructor. (or in a method that could be seen as equivalent to the constructor..)

I am branching this from 8.5 so that it can be merged both into 8.5 and 9.5.

donquixote avatar Oct 10 '21 18:10 donquixote

I did this directly from the github UI, so I don't have to clone and fork. Hope that's ok :)

donquixote avatar Oct 10 '21 18:10 donquixote

Psalm finds more of those. For now I am going to use @psalm-suppress PropertyNotSetInConstructor.

It seems to happen only with psalm error level < 3, which I am doing atm just to see what happens. Still, psalm is fully correct.

donquixote avatar Oct 10 '21 18:10 donquixote

Seems it is just those two. There are setter methods for each of them, but there is no guarantee (afaik) that they will run on object initialization.

In my own test class the following test method passes:

  public function testBasics() {
    self::assertNotNull($this->runTestInSeparateProcess);
    self::assertNull($this->backupStaticAttributes);
  }

So one of the is initialized, the other is not.

I think this PR is fine to make the docs reflect the actual behavior, without any risk. A better (but slightly more disruptive) fix might be to have those variables be false by default, instead of null. But I don't know enough about their usage, and the order of setup operations.

donquixote avatar Oct 10 '21 18:10 donquixote

Thank you for your contribution. I appreciate the time you invested in preparing this pull request. However, I did not merge it. Instead, I addressed the issue(s) in 07d34586b6ee7803c611b31e67408c8a8656d53a and d64ea7dbc7aa495c64fd035298302c48ce27be18.

sebastianbergmann avatar Jul 10 '23 06:07 sebastianbergmann

@sebastianbergmann Now I am a bit confused. I see some properties in there that are initialized as false, but you still add the ?bool in the doc comment. Perhaps you want to allow subclasses to set these to NULL?

donquixote avatar Jul 10 '23 09:07 donquixote

TBH, for the old 8.5 and 9.6 branches I do not want to think about this anymore and going forward will likely no longer make these types of changes.

sebastianbergmann avatar Jul 10 '23 09:07 sebastianbergmann