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

[v3] Fix Reactive Attribute Browser test #5879

Merged
merged 3 commits into from Jul 22, 2023
Merged

[v3] Fix Reactive Attribute Browser test #5879

merged 3 commits into from Jul 22, 2023

Conversation

luanfreitasdev
Copy link
Contributor

Review the contribution guide first at: https://laravel-livewire.com/docs/2.x/contribution-guide

1️⃣ Is this something that is wanted/needed? Did you create a discussion about it first?

2️⃣ Did you create a branch for your fix/feature? (Master branch PR's will be closed)
No
3️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out.
No
4️⃣ Does it include tests? (Required)
Yes

5️⃣ Please include a thorough description (including small code snippets if possible) of the improvement and reasons why it's useful.


This PR fixes the test name in the first browser test and improves the reactivity check in another test.

The first should check if the CannotMutateReactivePropException exception was thrown.

The second test verifies the reactivity.

Thanks for contributing! 🙌

@joshhanley
Copy link
Member

@luanfreitasdev thanks for the PR! Can you just confirm why the need for the PR? In my opinion, everything was covered in the original test and the new test is more verbose but not really testing any new functionality. Am I missing something?

@luanfreitasdev
Copy link
Contributor Author

hello!. The current first test expects an exception, which is not described in the test name. The second is just an improvement

@joshhanley
Copy link
Member

@luanfreitasdev no worries, can you just refactor it so that the first test is the success case and small like the original one, and then make the second test the exception test? Thanks!

@luanfreitasdev
Copy link
Contributor Author

Well noted. Thanks

@joshhanley joshhanley merged commit b358009 into livewire:master Jul 22, 2023
3 of 4 checks passed
@joshhanley
Copy link
Member

Thanks!

@luanfreitasdev luanfreitasdev deleted the fix-reactive-browser-test branch July 22, 2023 22:31
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.

None yet

2 participants