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

Update Tests workflow #265

Merged
merged 9 commits into from
Apr 28, 2024
Merged

Conversation

milwad-dev
Copy link
Contributor

The NativePHP supports Laravel 11, but the test workflow just runs on Laravel 10!

https://github.com/NativePHP/laravel/blob/main/.github/workflows/run-tests.yml#L17

@milwad-dev milwad-dev marked this pull request as draft April 2, 2024 10:39
@milwad-dev
Copy link
Contributor Author

@simonhamp
The failed tests are not about my changes

@milwad-dev milwad-dev marked this pull request as ready for review April 2, 2024 11:34
@simonhamp
Copy link
Member

This is a good update, thanks!

I think the tests are failing because there's a dependency on the npm server (the Electron API) running...

cURL error 7: Failed to connect to localhost port 4000 after 0 ms: Connection refused (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) for http://localhost:4000/api/window/open

Ideally, we wouldn't be trying to call the Electron API as part of these tests as that would increase the complexity of the tests dramatically.

You may be able to get them to run locally if you have an Electron-based NativePHP app on your machine that you can spin up before running these tests.

Overall I think we need to have the tests just mock the API calls.

I don't think that needs to be done as part of this PR tho.

@simonhamp simonhamp merged commit bb0f12a into NativePHP:main Apr 28, 2024
1 of 21 checks passed
@milwad-dev milwad-dev deleted the fix-run-tests-workflow branch April 28, 2024 18:48
@milwad-dev
Copy link
Contributor Author

So how do you know my changes or other changes are ok?

@simonhamp
Copy link
Member

simonhamp commented Apr 29, 2024

I have tested the suite locally with a NativePHP Electron API running and it all passes. I have reviewed this code carefully and it is fine (the actions get spun up correctly, it's only the Pest test suite that's failing).

Plus, the changes here only target the test suite itself, so I'm fine with merging this for now and fixing the underlying issues in another PR as it won't affect end users.

And even if it did, we're still in alpha, so things can break. It will be ok 👍🏼

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