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

Add test for controller names #1669

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

williamjallen
Copy link
Collaborator

CDash has a controller naming and designation convention, as discussed in #1587. Since then, I have forgotten to mark several new controllers as "final". I have added a test to enforce these rules automatically, rather than relying upon human reviewers to catch it.

As seen in the diff for this PR, this test is quite effective. It caught the fact that the controllers in the Auth directory were not marked with the "final" keyword.

Copy link
Member

@josephsnyder josephsnyder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per our F2F conversation:

There seems to be an issue with the second and third tests as changing the name of a valid class returns the following error:

test 10
    Start 10: /Unit/app/ControllerNameTest

10: Test command: /usr/local/bin/php "/cdash/vendor/bin/phpunit" "-c" "/cdash/phpunit.xml" "/cdash/tests//Unit/app/ControllerNameTest.php"
10: Test timeout computed to be: 1500
10: PHPUnit 9.6.4 by Sebastian Bergmann and contributors.
10: 
10: EPHP Fatal error:  Cannot declare class App\Http\Controllers\Auth\AVerificationController, because the name is already in use in /cdash/app/Http/Controllers/Auth/VerificationController.php on line 8
10: 
10: In VerificationController.php line 8:
10:                                                                                                               
10:   Cannot declare class App\Http\Controllers\Auth\AVerificationController, because the name is already in use  
10:                                                                                                               
10: 
10: 
10: In VerificationController.php line 8:
10:                                                                                                               
10:   Cannot declare class App\Http\Controllers\Auth\AVerificationController, because the name is already in use  
10:                                                                                                               
10: 
1/1 Test #10: /Unit/app/ControllerNameTest .....***Failed    0.28 sec

@williamjallen
Copy link
Collaborator Author

@josephsnyder This one is a tricky one to fix. The error you noted is caused by the autoloader expecting the file name to match the class name. I attempted to add a basic error message to hopefully point developers in the right direction in the rare case where this happens. I also deleted the test for this, since it's already being enforced by the autoloader anyway.

@josephsnyder josephsnyder added this pull request to the merge queue Aug 31, 2023
Merged via the queue into Kitware:master with commit d7da750 Aug 31, 2023
2 checks passed
@williamjallen williamjallen deleted the controller-name-test branch August 31, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants