Make assertSet() and assertNotSet() more strict

[DudeMason]

1️⃣ Is this something that is wanted/needed? Did you create an issue / discussion about it first?
Yes!
And No.

2️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out.
Nope!

3️⃣ Does it include tests, if possible? (Not a deal-breaker, just a nice-to-have)
Yes!

4️⃣ Please include a thorough description of the improvement and reasons why it's useful.
There are some properties in a few of our tests that we want to assert have been set false, 0, null, or ''.
Currently the code is using assertEquals() which is somewhat equivalent to using ==, whereas using assertSame() is equivalent to using ===, which is much more useful when deciding what a property is truly set to.
Currently $property could quite literally be set to 100 and doing assertSet('property', '1e2') would pass.

Testing the difference between the two is much more apparent when using assertNotSet() because the test would instead fail.

After applying the changes in this PR, the behavior between the two tests would be the reverse, of course.

One good example is, let's say, the property has been set to zero and we want to assert that the property isn't null.

Making this change would allow tests to be more tight-knit.
We have opted for using PHPUnit::assertSame($livewire->get('property'), $expectedValue) in the meantime.

5️⃣ Thanks for contributing! 🙌
Thank you!

P.S. Let me know if the expectException() isn't needed! :)
(assertNotSet is technically checking it)

[calebporzio]

Yeah, this makes sense, I think I would expect the same behavior. However, if we merge and push this, we might break lots of existing app's tests that rely on the == nature of ->assertSet.

Kind of a tough one, but I feel like this is a breaking change, so unless someone sternly objects and convinces me it's not I think I'm going to close it.

Again, it DOES make sense and I'm thankful for the contribution. Maybe for the next major version we can do this.

[DudeMason]

I totally understand! As soon as I saw a lot of strict type errors happen in the app's own tests, I figured it a slim chance that this would get approved.

I appreciate the consideration, though! Thanks! 😃

[joshhanley]

I wonder if we can create another method that does that, that's Livewire specific like assertSet is. Maybe assertSame? But maybe that's too confusing, so something like that?

Another option is a third parameter on assertSet, is a bool $strict or something?

[joshhanley]

Having a strict check would be good though. Have been bitten a few times just generally using assertEquals instead of assertSame.

[DudeMason]

Another option is a third parameter on assertSet, is a bool $strict or something?

Great idea!
Done! :)

[DudeMason]

Adding a strict flag resolves the issue of breaking existing tests and appeases the need for greater strictness in these functions.

Win - win!

[DudeMason]

Also, it seems like there's a bugged test that fails at random points and then passes in others, without really changing any code...

In \Tests\Browser\Alpine\Transition\Test.php::runThroughTransitions()

[joshhanley]

@DudeMason yeah some of the dusk tests and that test in particular can be flaky at times.

Nice! My only issue with it, is the third parameter doesn't mean anything to anyone reading the test.

I wonder if creating a assertSetStrict or something like that would be better.

Or even have both, and assertSetStrict can just use assertSet with the bool set to true under the hood?

[DudeMason]

@joshhanley Yeah, I agree with keeping the flag and adding the function using the flag. Because a lot of dev teams are used to functions with $strict flags, and their IDE's fill in the name of each parameter in small grey letters next to each provided argument, or something like that. It's really common to see $strict flags in code bases and libraries, so I think most developers are used to them.

But for those who like their functions to be descriptive about what they're doing, so you don't have to investigate before using them, we can add the descriptive function, like you stated, using the strict flag set to true.

Providing both options is nice. I can see some people who would stumble upon both and wonder why the more descriptive one was even necessary. But I can see a lot of people being grateful/happy that there's a function whose name says, more or less, exactly what it's doing.

[calebporzio]

Good thoughts here, I'd say let's just start with the extra boolean and add a full new method later if we want.

[DudeMason]

Any updates on this?

[joshhanley]

@DudeMason all looks ok to me. Will probably be reviewed when Caleb goes through PR's next.

[calebporzio]

Thanks @DudeMason!