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 cleanup - remove direct calls to BAO_Membership::add #22497

Merged
merged 1 commit into from
Jan 16, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Test cleanup - remove direct calls to BAO_Membership::add

Before

Direct BAO add called

After

API v4 used

Technical Details

Comments

@civibot
Copy link

civibot bot commented Jan 13, 2022

(Standard links)

@civibot civibot bot added the master label Jan 13, 2022
@demeritcowboy
Copy link
Contributor

Is the idea to deprecate BAO::add()? Because these tests are in a file called BAO, and most of the changes remove coverage of the BAO and are now covering API4.

Otherwise this seems ok.

@eileenmcnaughton
Copy link
Contributor Author

@demeritcowboy yeah - I think in general we want to stop calling BAO crud functions directly - even in tests

It does creat a file structure challenge - which is already doing my head in - ie do we test crud from apiv3 files , v4 files or BAO files - esp when testing both api versions

I kinda feel like we should move apiv3 crud to somewhere else because one day v3 api will die out - but the tests will still matter

@colemanw

@colemanw
Copy link
Member

I think it's ok for the tests to be updated to use the API.
I'm less clear about internal code; I think some code like a form pre/postprocess make sense to use the API, but very low-level stuff seems a little more dicey. But that's a different discussion.

@colemanw colemanw merged commit 44861be into civicrm:master Jan 16, 2022
@colemanw colemanw deleted the mem_ids branch January 16, 2022 02:30
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