ProductVariationTest::testTimestamps in the commerce submodule only contains two assertions on the time fields for product variation 1. Tests on the product variation migration entity should be done all together after a migration not split into two tests.
Ideally, the assertions on the time value should be in the assertProductVariationEntity in the test trait. But the time stamps can be changed by other migrations, #2754787: d6_term_node migration overwrites node changed timestamps. So, let's not go for a complete solution that works for every source.
Move the time stamp assertions to method testProductVariation.
Doit
N/A
N/A
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | 2904289-15.patch | 13.56 KB | heddn |
Comments
Comment #2
pbrown063 commentedIf I am understanding you correctly and not overthinking this should meet your aims.
Regarding your saying that:
Should product titles follow the same rule? I have a specific example available if this is the place for that discussion.
Comment #3
quietone commented@pbrown063, thanks for the patch! But I think you weren't working from HEAD, which is on d.o The change is to the tests in the submodule, commerce. That could probably be clearer in the IS, which I have updated.
Yes, post a migration we should check as many aspects of an entity in one test. What is your example?
Comment #4
quietone commentedMoved the timestamps tests to assertProductVariationEntity. That required changes to both the ubercart and commerce versions of ProductVariationTest.php, particularly the ubercart version. That was doing both the product variation migration followed by the product migration. The latter mmigration saves the product and modifies the changed timestamp to the current time, making the test fail. But this test is testing the product variation migration and should only be executing dependent migrations. So, changed that so it just executes the product_variation_type and then product_variation. But with those changes, the title and the product_id are no longer migrated. The title is a field and can be migrated when a deriver is used. Need to think about the product id.
Because the product migration modifies the changed time, NULL is used to avoid the assertion in the test.
Comment #6
quietone commentedOops, had a test version of the test file in the patch.
Comment #7
quietone commentedAnd the product id is NULL until filled in by the product migration. So this is good to go and the existing product variation migrations will behave and be tested the same.
Comment #8
heddnLooks good.
Comment #10
heddnComment #12
heddnThis caused HEAD to fail. It might just be we need a re-roll. https://www.drupal.org/pift-ci-job/795001
Comment #13
heddnHere's a re-roll.
Comment #15
heddnLet's get this re-roll right, ok?
Comment #17
heddn