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

NFC - minor Cleanup in test class #24827

Merged
merged 1 commit into from
Oct 27, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

NFC - minor Cleanup in test class

Before

After

Technical Details

Comments

@civibot
Copy link

civibot bot commented Oct 26, 2022

(Standard links)

@civibot civibot bot added the master label Oct 26, 2022
'street_address' => '1 Happy Place',
'city' => 'Miami',
'state_province' => 'Flordia',
'state_province' => 'Florida',
Copy link
Member

Choose a reason for hiding this comment

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

I suppose it could be interesting ("What happens when the state_province is wonky?")... but it's clearly not the focus of the test...

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol yes

@eileenmcnaughton eileenmcnaughton force-pushed the test_clean branch 2 times, most recently from caface6 to e74107f Compare October 26, 2022 23:29
$addFormat = '{contact.state_province}';
Civi::settings()->set('address_format', $addFormat);
$formatted_address = CRM_Utils_Address::getFormattedBillingAddressFieldsFromParameters($params, '99');
$this->assertTrue((bool) $formatted_address == 'AL');
$this->assertEquals('AL', $formatted_address);
Copy link
Contributor

Choose a reason for hiding this comment

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

so there seems to be trailing new line that is showing up here somehow @eileenmcnaughton

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm - the trailing line isn't new - the old assert permitted it. Ie in the screenshot the first assert passes & the second fails

I suspect it needed brackets around the comparison

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that's what formatted means...

And hence the previous code's (bool) to cast it true which then matched the string which autocasts to true so it always passed. How else do you deal with trailing newlines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@demeritcowboy so we could use that pattern to make all our tests pass....

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even simpler use the technique @mlutfy discovered: Automatic pass!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another winning technique

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah - not sure even @mlutfy's trick woudl have prevented this one

image

test this please

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@seamuslee001 seamuslee001 merged commit bf1dfac into civicrm:master Oct 27, 2022
@seamuslee001 seamuslee001 deleted the test_clean branch October 27, 2022 22:43
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