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

Fixed duplicate route creation issue in QuickStartCommand #127

Merged

Conversation

erhanurgun
Copy link
Contributor

Hello,

I've identified an issue in the QuickStartCommand and created this pull request to address it. Here are the changes I've made:

  1. In QuickStartCommand.php:

    • Added a new method called updateApiRoutes(). This method checks if the 'users' route already exists and only adds it if it doesn't.
    • This new method inspects the routes/api.php file and only appends the new route when necessary.
  2. Added QuickStartCommandTest.php:

    • Included a test method to ensure the command creates the correct files.
    • Added a test method to verify that API routes are updated correctly.
    • Included a test method to check that routes are not duplicated when the command is run multiple times.

These changes will ensure the command operates more reliably and reduce the likelihood of similar issues occurring in the future.

I'm looking forward to your feedback and review. If you have any questions or suggestions, please don't hesitate to ask.

Thank you!

- Add updateApiRoutes() method
- Implement logic to check for existing routes
- Add QuickStartCommandTest class
- Add tests to prevent duplicate route creation
@GautierDele
Copy link
Member

Hello @erhanurgun,

Thanks for your proposal, seems like a good idea
It seems that your tests arnt passing, could you have a look at it please ?

- Add testing environment variables to phpunit.xml
- Ensure api.php file exists before running tests
- Update QuickStartCommand to create api.php file if not exists
- Add proper test environment configuration in TestCase

These changes address the "File does not exist" errors in the CI pipeline and ensure a consistent test environment across different database configurations.
The updates should improve the reliability of the test suite and make it easier to run tests in various environments.
@erhanurgun
Copy link
Contributor Author

erhanurgun commented Jul 9, 2024

Hello @GautierDele,

Thank you for your feedback. I've reviewed the tests and made some adjustments to address the failing tests. Here's a summary of the changes I've made:

  1. In ./phpunit.xml:21:

    • Added environment variables for testing:
      <env name="APP_ENV" value="testing"/>
      <env name="DB_CONNECTION" value="testing"/>
  2. In ./src/Console/Commands/QuickStartCommand.php:109:

    • Updated the file creation logic:
      file_put_contents($routesPath, '<?php');
  3. In both ./tests/Features/Commands/QuickStartCommandTest.php:14 and ./tests/Feature/TestCase.php:14:

    • Added a check to ensure api.php exists before running tests:
      if (! File::exists(base_path('routes/api.php'))) {
          File::put(base_path('routes/api.php'), '<?php');
      }
  4. In ./tests/Feature/TestCase.php:28:

    • Added getPackageProviders and getEnvironmentSetUp methods to properly configure the testing environment.

These changes aim to resolve the "File does not exist at path .../routes/api.php" error that was causing the tests to fail.

I've pushed these updates to my fork. Could you please pull the changes and run the tests again? If you encounter any issues or have any questions, please let me know, and I'll be happy to assist further.

Thank you for your patience and collaboration.

Best regards,


Screenshots from the tests

fixed_ss
err_fixed_ok

tests/Feature/Commands/QuickStartCommandTest.php Outdated Show resolved Hide resolved
tests/Feature/TestCase.php Outdated Show resolved Hide resolved
phpunit.xml Outdated Show resolved Hide resolved
tests/Feature/TestCase.php Outdated Show resolved Hide resolved
@GautierDele
Copy link
Member

I'm gonna have a quick look later this afternoon in order to test it on my side, thanks for your time
I'm gonna also fix the problem i gave above, so you don't need to 😉

@GautierDele
Copy link
Member

Thanks @erhanurgun merging !

@GautierDele GautierDele merged commit c5d525f into Lomkit:master Jul 10, 2024
16 checks passed
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