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

Adds basic test suite for existing functionality #8

Open
wants to merge 1 commit into
base: 1.0
Choose a base branch
from

Conversation

ksassnowski
Copy link

This PR adds a basic test suite for the existing functionality. Since I'm not too familiar with Livewire, I didn't add any tests using Livewire::test(). I also tried to mostly document the existing behavior without changing too much of the implementation.

There were a few issues I noticed while writing the tests:

  • The execute method on the Spotlight class does not check if getCommandById actually found a command. If it returns null the method_exists check will fail with a TypeError.
  • The placeholder on SpotlightCommandDependency doesn't have a default value but also isn't required in the constructor. This effectively makes it mandatory to call setPlaceholder since you'd run into an error when calling toArray because the $placeholder property has not been initialized yet.
  • The setType method on the SpotlightCommandDependency class accepts any string, even though it looks like the package only knows how to deal with search and input. Maybe replace it with two separate methods to set the $type to search or input, respectively?

Anyways, let me know what you think!

@ksassnowski ksassnowski mentioned this pull request Apr 23, 2021
@PhiloNL
Copy link
Contributor

PhiloNL commented Apr 23, 2021

Thanks @ksassnowski - Really appreciate the time you took to make improvements and write the tests🙌 I'll take a look this weekend or Monday and get back to you! Have a great weekend!

@PhiloNL
Copy link
Contributor

PhiloNL commented Apr 29, 2021

Sorry for the wait @ksassnowski, it's still on my list to review, thanks for your patience 🙏

@ksassnowski
Copy link
Author

No worries 🤘

@Krato
Copy link

Krato commented Aug 8, 2022

Updates?

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.

3 participants