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

Version check test: in alphas don't test for site stats not provided for alphas #11831

Merged
merged 2 commits into from
Mar 19, 2018

Conversation

agh1
Copy link
Contributor

@agh1 agh1 commented Mar 19, 2018

Overview

The version check test is failing on master because it asserts that certain stats should be there, but they aren't generated for versions with alpha in the name.

Before

The test CRM_Utils_versionCheckTest::testGetSiteStats will fail when run on an alpha

Failed asserting that an array has the key 'hash'.

After

The test will not assert things that aren't true on an alpha.

Technical Details

This proposed change sidesteps the question of whether an alpha should have these various statistics generated in CRM_Utils_VersionCheck::getSiteStats. However, if anything changes over there in the condition, it should get a corresponding change in the test.

Comments

See the thread starting with this post on Mattermost

@totten
Copy link
Member

totten commented Mar 19, 2018

@agh1 I like this for a quick fix.

Aside: I made an edit because the test was still failing -- the conditional needed to be set so that the assertions run if alpha is not part of the string.

@totten
Copy link
Member

totten commented Mar 19, 2018

Review notes: the PR is framed as a correction to a test suite. It has no implications for runtime behavior, user experience, or integrations.

Merge on pass.

@agh1
Copy link
Contributor Author

agh1 commented Mar 19, 2018

Ha! Thanks for getting my logic right, @totten !

@eileenmcnaughton eileenmcnaughton merged commit 410a666 into civicrm:master Mar 19, 2018
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.

4 participants