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

Inconsistent configuration tests #181

Closed
roband7 opened this issue Apr 17, 2017 · 3 comments
Closed

Inconsistent configuration tests #181

roband7 opened this issue Apr 17, 2017 · 3 comments
Labels
Code Quality Enhancement Outdated Issue is no longer relevant due to significant changes in the codebase

Comments

@roband7
Copy link

roband7 commented Apr 17, 2017

I'm assuming the intention is that he implementation and 'owner' of these tests is classes/ConfigurationTest.php

The problem is that a lot of the knowledge of test result array keys, and all the error texts, are duplicated in controllers/admin/AdminInformationController.php and install/controllers/http/system.php

Right now system.php seem to be in sync with ConfigurationTest.php, but AdminInformationController.php isn't.

The need for syncing and the duplication of error texts is silly and error-prone.

My suggestion is to move all the logic and and texts over to ConfigurationTest.php and slim down AdminInformationController.php and system.php

Also, if you do a command-line install using install/index_cli.php it doesn't call ConfigurationTest.php at all, bypassing all the tests! Yes, I learned this the hard way, and was why I started digging into this mess :)

@firstred firstred added this to the 1.0.1 milestone Apr 19, 2017
@firstred
Copy link
Contributor

Thanks. It is indeed something we want to take care of, but it currently requires too many changes for a patch version. Rescheduling for 1.1.0 instead.

@firstred firstred modified the milestones: 1.1.0, 1.0.1 Apr 20, 2017
@firstred firstred modified the milestones: 1.0.2, 1.1.0 Jun 3, 2017
@firstred firstred self-assigned this Jun 28, 2017
@firstred
Copy link
Contributor

Just a basic solution for now, not very verbose: e08c47c

@firstred firstred modified the milestones: 1.1.0, 1.0.2 Jun 28, 2017
@Traumflug Traumflug removed this from the 1.1.0 milestone Oct 22, 2018
@Traumflug
Copy link
Contributor

This area received a lot of smaller improvements over the last year. AFAIK the only issue left is the distribution of tests and their messages over several places. Installer, AdminInformationController, ConfigurationTest and last not least the place where all this should go: module Core Updater with its AdminCoreUpdaterController.

@getdatakick getdatakick added the Outdated Issue is no longer relevant due to significant changes in the codebase label Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Quality Enhancement Outdated Issue is no longer relevant due to significant changes in the codebase
Projects
None yet
Development

No branches or pull requests

4 participants