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

[TEST] add assert to ensure nullArray & nullObject are not contaminated #14543

Merged
merged 2 commits into from
Jun 17, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jun 14, 2019

Overview

Test fix to ensure the nullArray & nullObject are not being contaminated during test runs

Before

Less testing

After

More testing

Technical Details

After deep & excruciating pain I found an intermittent test fail was caused by contamination of these 'helpful helpers' - search & destroy similar cases

Comments

@civibot
Copy link

civibot bot commented Jun 14, 2019

(Standard links)

@civibot civibot bot added the master label Jun 14, 2019
@seamuslee001
Copy link
Contributor

Test fails also relate i think @eileenmcnaughton

@@ -482,6 +482,8 @@ protected function tearDown() {

$this->cleanTempDirs();
$this->unsetExtensionSystem();
$this->assertEquals(NULL, CRM_Core_DAO::$_nullArray);
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton i think this should be [] not NULL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seamuslee001 looks like jenkins does too :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets see what jenkins thinks now - kinda likely this won't pass

@seamuslee001
Copy link
Contributor

added MOP as change makes sense to me

@eileenmcnaughton
Copy link
Contributor Author

Lol yeah -not counting on that pass though!

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@eileenmcnaughton
Copy link
Contributor Author

So the interesting thing is I would expect the first test to fail to fail in isolation too - but for me it doesn't... unless I got it wrong

Using less different types of exceptions is cleaner- our own exception is pretty much our go-to
so using here
@eileenmcnaughton
Copy link
Contributor Author

test this please

2 similar comments
@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 woot!

@eileenmcnaughton eileenmcnaughton merged commit 3c8861b into civicrm:master Jun 17, 2019
@eileenmcnaughton eileenmcnaughton deleted the wtf_null branch June 17, 2019 14:56
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