Skip to content

Refactor set and reset of capabilities#29195

Merged
phil-davis merged 1 commit into
masterfrom
refactor-integration-test-before-after
Oct 10, 2017
Merged

Refactor set and reset of capabilities#29195
phil-davis merged 1 commit into
masterfrom
refactor-integration-test-before-after

Conversation

@phil-davis

@phil-davis phil-davis commented Oct 10, 2017

Copy link
Copy Markdown
Contributor

Description

  1. wasCapabilitySet was supposed to return a proper boolean - fixed.
  2. Because of that, the comparison if ($savedState !== $testingState) was always true because the types were different. Item (1) makes sure that $savedState is a boolean.
  3. After fixing that, it meant that the test condition was happening. So when it seems that the capability is already set correctly, there is no "wasted" attempt to set it again. Great in theory!

But in practice, enabling some capability settings will also change sub-settings. e.g. when files_sharing api_enabled is switched on, then various sub-capabilities get set to their defaults. So the original saved state of capabilities is not a good indicator of what is currently set/not set. There were then some failures, particularly in the capabilities and sharing suites.

To fix that in the general case, you would have to re-query the capabilities after every change you make. That defeats the purpose of trying to be efficient and do a single capabilities query, then only change the things that need changing. It turns out to be just as quick to set every capability, whether it is already set or not.

  1. Save the capabilities that really do get changed as an array. At the end of the scenario these can be put back. It eliminates needing individual restore* methods that know what to put back. And eliminates the need for the (new) restoreAppConfigs() method.

  2. Rename setupAppConfigs() back to resetAppConfigs() - along with item (4) this reverts the abstract methods in AppConfiguration.php back to the way they were previously. It means that existing integration tests in apps that use BasicStructure and AppConfiguration do not get broken (i.e. do not have to all be patched)

Related Issue

This is a follow-on from PR #28970

Motivation and Context

How Has This Been Tested?

Extra test runs in Jenkins in temporary PRs to make sure it works and does not take too much time.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@phil-davis

Copy link
Copy Markdown
Contributor Author

The original PR is not "broken" in an outward way. The unexpected "always true" at point 2 was hiding an underlying issue.

And also, making these refactorings also helps avoid needing to touch the integration tests in apps.

@phil-davis

Copy link
Copy Markdown
Contributor Author

Backport stable10 #29200

@phil-davis phil-davis merged commit 1155cce into master Oct 10, 2017
@phil-davis phil-davis deleted the refactor-integration-test-before-after branch October 10, 2017 15:31
@lock

lock Bot commented Aug 2, 2019

Copy link
Copy Markdown

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock Bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants