Refactor: Remove side effects from data provider #7478
Conversation
|
@pulsovi thanks for the contribution! It appears our older automated tests are failing due to this change probably due to implementation update. https://github.com/impress-org/givewp/actions/runs/10254681539/job/29133510148?pr=7478 |
pulsovi
force-pushed
the
fix/test/test-misc-functions
branch
from
8e837fc to
53407f1
Compare
- Eliminated side effects in test_give_meta_helpers data provider - Ensures consistent test environment regardless of --filter usage This change prevents unexpected failures when running specific tests with PHPUnit's --filter option.
double declaration of `add_filter('..._post_meta')` breaks the return
value. See test: Tests_MISC_Functions::test_give_meta_helpers
MyIsam engine does not support transactions
pulsovi
force-pushed
the
fix/test/test-misc-functions
branch
from
53407f1 to
30a6faf
Compare
There was a problem hiding this comment.
@pulsovi it looks like this PR has expanded from fixing our legacy test suite side effects to modifying code in production. Please understand this introduces risk to this PR and is something we have to be very careful about as it could impact our customers.
What exactly are you hoping to solve it here?
| @@ -49,7 +49,7 @@ public function run() { | |||
| created_at DATETIME NOT NULL, | |||
| updated_at DATETIME NOT NULL, | |||
| PRIMARY KEY (id) | |||
| ) $charset"; | |||
| ) $charset ENGINE=InnoDB;"; | |||
There was a problem hiding this comment.
What's the purpose of adding this?
There was a problem hiding this comment.
On my machine where the default engine is MyISAM, many of the tests crash because of their interactions.
Indeed, the teardown cleanup function of legacy tests uses SQL transactions with ROLLBACK to undo any modification of the DB by the test.
But MyISAM does not manage transactions...
This is why I suggest explicit use of the InnoDB engine.
There was a problem hiding this comment.
@pulsovi thanks for the clarification
| 'donors_db' => give()->get(Give_DB_Donors::class), | ||
| 'donor_meta_db' => give()->get(Give_DB_Donor_Meta::class), | ||
| 'comment_db' => give()->get(Give_DB_Comments::class), | ||
| 'comment_db_meta' => give()->get(Give_DB_Comment_Meta::class), | ||
| 'give_session' => give()->get(Give_DB_Sessions::class), | ||
| 'formmeta_db' => give()->get(Give_DB_Form_Meta::class), | ||
| 'sequential_db' => give()->get(Give_DB_Sequential_Ordering::class), | ||
| 'donation_meta' => give()->get(Give_DB_Payment_Meta::class), |
There was a problem hiding this comment.
What's happening here?
There was a problem hiding this comment.
All of these classes are extensions of Give_DB_Meta.
However, this class, during its instantiation, will override all post modification methods via hooks such as:
if (in_array('delete_post_metadata', $this->supports)) {
add_filter('delete_post_metadata', [$this, '__delete_meta'], 0, 5);
}This will trigger a bugged behavior if one of these classes is instantiated more than once.
In the specific case of deletion, the return value indicates the number of rows deleted from the database.
But if the hook is called by several instances of the class, this is what will happen:
- The first hook will do the deletion work and return the number of rows deleted
- The second hook will look for lines to delete, find none (they were all deleted by the first hook) and therefore return the value
0 - The hook ends by systematically returning
0, the return value is wrong.
For this reason, I made the minimum possible modifications to approach singleton pattern behavior.
This is the code you present here.
There was a problem hiding this comment.
🤔 interesting!
I think the singleton pattern here does make sense 👍
We'll just have to run it through QA internally to make sure everything works as expected.
|
@jonwaldstein Hi, I'm not very familiar with the PR treatment process. What should I do here next? |
|
This PR is stale because it has been open 45 days with no activity. Stale PRs will NOT be automatically closed. |
|
@pulsovi thanks for your patience, we often have high priority work internally and can be hard to find time for addressing open source contributions. That is a balance we are actively trying to figure out 😅 . I'm going to send this through our dev and QA team, if they find any issues I may ask for an update to the PR to address. Thanks again for this PR, we really appreciate your effort 🙌 |
There was a problem hiding this comment.
@pulsovi I spoke with our dev team. We still have some reservations about merging this PR.
It seems the scope of changes in this PR grew from updating our legacy unit tests data provider to modifying production (once our CI started failing).
Since we have not encountered any of these side effects in production (no customer reports) outside of the legacy unit tests that you have brought to our attention we would like to raise the question if modifying production is actually necessary?
Something to note is we have actually deprecated the Give_Unit_Test_Case as of GiveWP 2.22.1 which makes me wonder if some of these tests were using our newer Give\Tests\TestCase if these side effects would still exist.
To help us further explore I have a couple questions for you:
- Have you encountered any of these issues outside of unit testing?
- Could these issues you encountered perhaps be resolved within the unit testing framework itself without modifying production?
Once again, thanks for your contribution and patience 🙌
Issue
Tests_MISC_Functions->test_give_meta_helpers's data provider had side effects, which caused unexpected failures when using the --filter option of PHPUnit.Solution
Impact
This change improves test reliability and isolation, allowing more predictable execution, especially when using --filter.
Good practice
Data providers should never have side effects to guarantee the consistency and independence of the tests.