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] [REF] [TEST] [EXPORT] Update various export tests to test csv output with new functions #14780

Merged
merged 3 commits into from
Jul 10, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jul 10, 2019

Overview

Updates some unit tests on export functionality to use preferred methodology

Before

Testing created temp table

After

Testing csv output

Technical Details

Because he is wiser than all of us @totten pointed out that we could throw an exception in the civiExit process for unit tests. This, combined with shipping phpleague csv package gave us a way to test the actual output. This upgrades some tests to do that. Testing thee csv output rather than the temptable details is preferred because the temp table is 'the workings' and in many cases would be better not created at all. Testing the columns created was REALLY useful when we were refactoring that but I don't believe removing that chunk now loses us any coverage

Comments

@civibot
Copy link

civibot bot commented Jul 10, 2019

(Standard links)

@civibot civibot bot added the master label Jul 10, 2019
@eileenmcnaughton eileenmcnaughton changed the title Export test2 [NFC] [REF] [TEST] [EXPORT] Update various export tests to test csv output with new functions Jul 10, 2019
@eileenmcnaughton
Copy link
Contributor Author

I think this is unrelated

@eileenmcnaughton
Copy link
Contributor Author

@colemanw I think this is mergeable

@colemanw
Copy link
Member

retest this please

Note we are implictly checking headers so no need to do so separately
The move from testing sqlColumns to csv output is deliberate. While refactoring, cleaning up the sqlColumns stuff
having test on it was VERY helpful. But, at the end of the day the csv IS the output and testing that allows
us to improve the mechanism to develop that output
@eileenmcnaughton
Copy link
Contributor Author

@colemanw this . is passing now

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 wanna merge this - should be a no brainer as it's tests only & it passes

}
}

$this->assertEquals([
'billing_im_provider' => 'billing_im_provider text',
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 just wonder are we loosing anything with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well we are losing the workings of the function - ie what sort of fields it writes to the temp table. That felt very important when cleaning up that part of the function but I aspire to change the way in which it writes to the temp tables in future (ie either not write to them when not required or just write the id & transform on output) so I want to switch to testing the outcomes rather than the internals

@colemanw colemanw merged commit 5a6a671 into civicrm:master Jul 10, 2019
@colemanw colemanw deleted the export_test2 branch July 10, 2019 23:49
@colemanw
Copy link
Member

colemanw commented Jul 10, 2019

This PR title wins the Most Acronyms in Square Brackets award 🏆

@eileenmcnaughton
Copy link
Contributor Author

My life is fulfilled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants