Closed (fixed)
Project:
Rules
Version:
8.x-3.x-dev
Component:
Tests
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
11 Dec 2021 at 08:34 UTC
Updated:
13 Feb 2022 at 23:19 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
tr commentedComment #3
tr commentedThis would be disruptive to commit at the moment, because of all the other outstanding patches that modify these same test functions.
Comment #4
jonathan1055 commentedHow about commiting just the first two hunks, which only alter
setUp()? Then when working on these tests in other issues, the changes can incorporate the use of the new stored properties. There will probably never be a time when there is nothing conflicting with other patches. So having this issue postponed may mean it gets forgotten. Just an idea ...Comment #5
tr commentedThere are other things I'd like to fix but haven't opened the issues yet. For example, static analysis complains about calling User methods on variables typed as AccountInterface. That one will touch every test case.
I think at some point we're just going to have to schedule a day to commit all outstanding test changes, so that we only have to re-roll affected patches once instead of dealing with the changes one-by-one.
Also I would like to look into refactoring this test in some way so that it doesn't have to be modified for every single patch.
Comment #6
jonathan1055 commentedOK I see.
Yes that would be good. When #3255074: Improve ConfigureAndExecute test is done it will remove the outstanding test changes from the Select List issue. So it would be good if you could do that first.
Comment #7
tr commentedOK, let's try that for now. Here's the patch.
Comment #8
tr commentedLet's also fix the datatype for $account, as this is wrong as per static analysis. It shouldn't be AccountInterface, it should be UserInterface. We're doing lots of things like
$this->account->addRole('test-editor'), but you can't add a role to an AccountInterface.Comment #10
tr commentedCommitted #8, leaving open to remind us to clean up the test cases as per #4.
Comment #11
jonathan1055 commentedHere's a patch which uses the new properties in all the remaining tests within the class. I have checked the issues which I've been working on, and this will not cause any clashes which require existing patches to be re-rolled. Specifically it will not cause any clashes in
#2471481: Integrate Typed Data Widgets
#2664280: Select lists in action & condition configuration forms
#2723259: Allow single-valued data selector input to be passed as an array for 'multiple' context fields
#2816157: Simplify UserHasRole::doEvaluate()
#3059402: Set data action in referenced entity.
#3103808: ConfigureAndExecuteTest creates malformed Rules
#3110593: Add conditions for Date Check and Date in Range
I have not gone through every 8.x 'needs work' or 'needs review' issue, but I don't think there are many others which alter that test file. If you know of others tell me. But now seems a good time to commit this.
Edit: Actually I have found one issue #3259453: Rename all Rules events which will clash with this patch. But it is easy to fix.
Comment #13
tr commentedOK, looks good. Thanks. Committed #11.