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] Add in unit test to lock in the fix for the is_deleted in where… #20733

Merged
merged 1 commit into from
Jul 5, 2021

Conversation

seamuslee001
Copy link
Contributor

… clause issue

Overview

This adds in a unit test specifically for the is_deleted field to lock in the fix in #20724

Before

No unit test

After

Unit test

ping @demeritcowboy @colemanw

@civibot
Copy link

civibot bot commented Jun 30, 2021

(Standard links)

@civibot civibot bot added the master label Jun 30, 2021
@seamuslee001 seamuslee001 force-pushed the is_deleted_unit_test branch from 8361dbc to a4116a5 Compare June 30, 2021 21:56
@colemanw
Copy link
Member

colemanw commented Jul 1, 2021

I really don't like tests which directly use the Api4SelectQuery object. That class is for internal use only and we should be able to refactor it without breaking brittle tests which expect it to always stay the same.

Note that the ContactApiKeyTest uses the api instead.

… clause issue

Move Unit test into its own class and use normal apiv4 wrapper rather than internal class as per Coleman
@seamuslee001 seamuslee001 force-pushed the is_deleted_unit_test branch from a4116a5 to e1f5526 Compare July 5, 2021 03:17
@seamuslee001
Copy link
Contributor Author

@colemanw have moved it now and also changed it as you have suggested can you check again please

@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

@colemanw colemanw merged commit 86a9a21 into civicrm:master Jul 5, 2021
@colemanw
Copy link
Member

colemanw commented Jul 5, 2021

Thanks for updating it @seamuslee001. Looks good.

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