Problem/Motivation
The current field assertions are not fully compatible with the Simpletest assertions. This is needed to make the largest possible amount of WebTest conversions pass with simple conversions.
Example of incompatibility:
The current assertFieldByName()
and assertNoFieldByName()
both only check 1 field on the AssertLegacyTrait class. The old Simpletest method tested all fields on the expected value.
This causes problems when checking on a field that is on the page more than one time, the op field for example.
Second example:
For checkbox fields the current assertFieldById()
passes when using a second parameter value of the empty string ''
for 'ignore the value, just check if the field exists'. This behavior is incorrect and if you want to ignore the checkbox value the second parameter must be NULL
not an empty string.
Proposed resolution
Change the implementations on AssertLegacyTrait to work similar to the implementation of Simpletest.
Remaining tasks
This issue had been postponed, waiting on #2876357: Incomplete test coverage on AssertLegacyTrait field assertions
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#93 | 2868019-89.patch | 17.46 KB | Lendude |
#89 | 2868019-89.patch | 17.46 KB | Lendude |
Comments
Comment #2
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedWork in progress, just a first attempt and doesn't work yet.
Comment #3
dawehnerIdeally we would have some test case first. You know, this kinda gets you the freedom to experiment a bit more :)
Comment #4
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedTest driven development, sound like a plan!
Patch now only contains the test, won't even try a needs review because it will fail by design.
It actually failed where I did not fully expect it, it cannot find any field when more are on the page. I'll experiment on it and come up with something that will pass.
As this might be a blocker for many other conversions and a bug in a much used Trait, should it be Major or is normal ok?
Comment #5
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedWell, maybe a needs review after all to prove it will fail..
Comment #7
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedThis kind-of escalated a bit, please tell me if I've been seeing this too big. All the field assertions were actually not fully compatible with the old versions. All were converted to use Mink assertions which in the basis is fine but because of the changes in how Mink sees a field (buttons are not fields) there are incompatibilities. The $value handling in the new methods were not the same either.
To make it as similar as possible I reverted the methods back to the original Simpletest versions (has to introduce constructFieldXpath() for that).
I wrapped the assertions in assertFieldByXPath() and assertNoFieldByXPath() to be able to throw the same exceptions as other assertions are.
Another thing I found out that the assertion in BrowserTestBaseTest:
Was incorrect, it checked 'name' while it should have checked 'edit-name', making the test pass while it shouldn't have.
test-only should fail, the normal patch should not ;)
Comment #8
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #10
LendudeI quite like this approach
It looks like these methods don't have test coverage currently, so I think we need to add that if we are making changes to them
Needs a full path right?
Comment #11
dawehnerJust a general advice. If you realize this also tries to expand the issue title and issue summary. This helps people to figure out what is going on.
This should use a fully qualified class name (FQCN), aka a starting slash ;)
I don't see test coverage for this change to be honest.
Comment #12
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedThanks for the reviews, I'll work on the test coverage and make it complete.
On the changing of title and summary, I did it the first time and then accidentally reloaded the page. Did update the IS the second time but forgot the title, sloppy me..
Upping to Major because it is blocking a number of conversions.
Comment #13
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedRight, quicker than expected.
10.1: Added test coverage for these assertions
10.2 & 11.1: Added the namespace
11.2: Added extra test coverage for this one. It was already indirectly covered by
$this->assertNoFieldById('edit-name', NULL);
because this assertion would fail without the fix but this does seem better and more complete.Comment #14
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedSorry, code reshuffle not completely succesfull, new patch and diff.
Comment #15
LendudeAll changed assertions now have coverage and this matches nicely with the #2807237: PHPUnit initiative directive of "Improve AssertLegacyTrait for maximum compatibility with SimpleTest."
Comment #16
dawehner+1
Comment #17
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedSorry about this, needed to set it back because it's still not 100% compatible as I just found out. The assertFieldsByValue() method doesn't work for checkbox fields yet.
Comment #18
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedHad to rewrite some parts for the checkboxes but it's working on all tests.
Change:
- Changed the assertNoFieldByXpath to use the assertFieldValues() method to make it behave the exact same way.
- Added support for a checkbox field
- No custom exceptions where not needed anymore (previous addition in this patch). The exceptions differ but as they're normally not that relevant I don't see problems with it. Negative assertions in the test did need to catch multiple types of exceptions though.
- More testcoverage, all field methods are now covered in both positive and negative situations.
Tested with the comment conversion and the latest additions make another test work again.
Comment #19
dawehnerinterdiff-14-18.txt 6.66 KB
The interdiff of evilness!!11!
Let's not sure FormattableMarkup, but rather just sprintf. There is no reason to render this message as HTML output, IMHO
I'm wondering in general whether
===
is more what we want here. Any thoughts?It is a bit weird, could this internally use
findField
already? I'm also wondering whether adding the@trigger_error
here would make sense, at least for now everything would cause errors, wouldn't it?This is some serious test coverage over here
Comment #20
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedThanks for the feedback, hope to be able to post a patch today.
Interdiff actually wasn't garbage, I just changed a lot ;)
19.1: Copy paste, agree though and will change it
19.2: Yup, missed that one, will change it to ===
19.3: I think we need to update the deprecation message. findField is not compatible with the old method because it doesn't find buttons. I'll expand it with the correct method for finding buttons.
19.4: Couldn't make it complete with less. I do think it's 100% now :)
Comment #21
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedFixed everything in the patch. I updated all the deprecation warnings to correctly refer to the button methods as well as the alternative is either the field or button method.
Comment #22
dawehnerThank you @michielnugter
I think we should be good here ...
Comment #23
dawehnerThis work is readdy for me
Comment #24
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #26
LendudeFailed due to #2868880: Random fails in ContextualFilterTest::testAddContextualFilterUI, back to RTBC
Comment #28
alexpottCommitted and pushed e918af6 to 8.4.x and 3ef5c84 to 8.3.x. Thanks!
Unused use fixed.
Fixed typo on commit.
Comment #31
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedThanks @alexpott for getting this one in! This will save us a lot of work I think on the rest of the conversions.
Comment #32
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThanks for fixing this, but it may cause some existing passing tests to fail as I found out yesterday. For other contrib maintainers who did not see this coming and suddenly found their tests failing when previously they passed, I am sharing what I have discovered about the change committed on 30th April.
When the field is a checkbox and you previously did not care what value the field had, just that existed, in
assertFieldById()
a second parameter value of empty string''
would pass the tests, but that now fails. You need to specifyNULL
for the $value instead. The same applies toassertFieldByName()
,assertNoFieldById()
andassertNoFieldByName()
. See #2874410: Tests with assertFieldById fail to find field at 8.3 and 8.4, ok at 8.2 for more.I have added this example in the summary to help other find this issue.
Comment #33
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedRight, good point @jonathan1055, didn't really saw this one coming.
I think we should add a change record.
Comment #34
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedDraft posted:
https://www.drupal.org/node/2874609
Can someone review and publish if it's OK?
Comment #36
alexpottGiven #32 I've reverted this change we shouldn't be breaking tests in a patch release. @jonathan1055 thank you for the report and sorry for the extra work.
Comment #37
alexpottComment #38
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI would like to fix the issue with as little deviation from the original methods as possible.
The main problem I think is that the value is now tested stricter than before for checkboxes. Do you agree on this @jonathan1055 or are there more problems that you can see?
This change could work.
Instead of
We could do:
This would ignore the checkbox value test for checkboxes if not passed specifically as a boolean while keeping the behavior intact for non-checkboxes.
The is_bool check really needs to go inside the if because otherwise the following code would be triggered and still cause a fail:
Will try to make a patch of it soon.
Comment #39
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedDon't be lazy.. Now with patch :)
Includes the on-commit fixes.
Comment #40
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedAs @alexpott suggested on IRC, needs additional testing on the checkboxes
Comment #41
dawehnerEven conceptually a boolean is kinda what you expect, right?
We should have some kind of test coverage for that.
Comment #42
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented@alexpott from #36
It was not too much work because I had enabled daily testing on my module at 8.2, 8.3 and 8.4 and noticed immediately when it began to fail, so I only had one day of git log to search to find this issue. :-)
True, but the tests which now break were only passing by fluke, i.e. they were coded incorrectly with
''
as second parameter, but did not fail due to an incorrect==
check. The documentation for AssertContentTrait::assertFieldById clearly states thatNULL
should be used for ignore the value so it was our fault that the test suddenly failed.@michielnugter in #38
Yes I agree.
This might be making it worse and more tests fail. I have just verified this existing behavior: a checkbox that is checked is found and passes with second parameter as NULL, TRUE, '1' or 1. A checkbox that is not checked is found with second param NULL, FALSE, '' or 0 (but fails with '0').
It is my understanding that the AssertLegacyTrait has been introduced to make it easier to switch from WebTestBase to BrowserTestBase in D8. These old assertions are deprecated and will be removed in D9 so I suggest that the code for finding checkboxes is not treated as a special case (even though it is currently a loose test) so that test results will remain as now.
So I think I have argued myself round to the point that alexpott made, that we should not be breaking existing test results, particularly for those who have already made the conversion to BTB. However, the rest of the fixes in patch should still be committed, to allow progress on conversion.
Comment #43
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI've done some more research into the compatibility and found the following:
This is the Simpletests' first if statement in assertFieldsByValue.
So actually my improvement to make the checkbox assertion actually work, make it break BC: go figure :)
New patch reverted the patch to make the checkbox work correctly. I kept 1 change in the assertFieldsByValue because it fixes another bug I would like to get in. It incorrectly checked the text for all elements, including input fields. They don't have text and it made certain assertions fail.
Might still need test coverage added but I'm not sure yet. Posting the patch now to have something.
Comment #45
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedTest fail makes sense, it tested the correct checkbox behavior. I dropped some of the tests parts to make it pass.
Also removed an accidental R.
Comment #46
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThe two tests which caused the fails in #43 are perfectly valid, and should remain. Therefore there could be something not quite right in the changes being made to AssertLegacyTrait.php.
$this->assertFieldByName('checkbox_disabled', FALSE);
is a valid test, and should pass with the existing code and any future change. Likewise$this->assertNoFieldByName('checkbox_disabled', TRUE);
is valid and should pass. I have tested these with the existing AssertLegacyTrait.php code and they both pass.I think that before any code is changed in AssertLegacyTrait.php we need to improve the test coverage. That way we can ensure we do not inadvertantly introduce changes which cause existing contrib module tests to fail, such as happened in #29 and #30 and which had to get reverted in #35. Or if we do make changes which cause existing tests to fail then we will know about them in advance, and make decisions about how to control the impact. I know that some of the current assertion code is a bit loose, so we will not be able to write strict tests, but mostly they do work with the correct parameters. There are also the tricky cases where the current assertions pass even when they should not (such as the example I found in #32. Not sure how we should deal with that yet.
Here is a patch which adds coverage for assertNoField(), assertField(), assertNoFieldById(), assertFieldById(), assertNoFieldByName(), assertFieldByName(), assertFieldChecked(), assertNoFieldChecked() and the specific tests on checkbox fields.
Comment #47
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedSmall updates from #46:
testLegacyFieldAsserts()
into three smaller more managable functions, as it naturally divided intotestLegacyXpathAsserts()
,testLegacyFieldAssertsWithTextfields()
andtestLegacyFieldAssertsWithNonTextfields()
Comment #48
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedGreat idea to add proper test-coverage first, should have started with that before even thinking about tinkering with these assertions :)
I have split it off into a separate issue to keep this issue focussed on fixing the assertions.
Issue: #2876357: Incomplete test coverage on AssertLegacyTrait field assertions
I have added some more testcoverage and will post the follow-up patch soon.
Postponing this issue for now.
Comment #49
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedAnd the test coverage is in! Let's get this to work.
Comment #50
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI updated the assertions and added even more test coverage to cover the issue #2874410: Tests with assertFieldById fail to find field at 8.3 and 8.4, ok at 8.2. Based it on #45 as the next ones were test-coverage only.
Interdiff is hard to read because of the test coverage getting in.
Comment #51
LendudeWe now have passing test coverage for what caused this to be reverted, back to RTBC
Comment #52
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedGreat work michielnugter and Lendude, on this issue and the test coverage one.
I have been away for a while so only just catching up. Putting this back to 'Needs Review' as I am not totally sure of the statement "We now have passing test coverage for what caused this to be reverted" and I want to do a full check/review before this is RTBC. Sorry to hold it up.
Jonathan
Comment #53
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedGenerally, this looks very good. Here are a few points of review:
Thanks for adding this. It shows that the previously sloppy passing of tests is not tightened by your changes, which is exactly what we wanted (as that made the existing contrib tests fail and caused the commit to be reverted). For completeness, though, we should also add the same two assertions for
$this->assertFieldById()
as that is actually the assertion which I had the problem with.with
For clarity, either use NULL for the second parameter of both calls, or do not use any second parameter in either. It looks like there is a purpose in having them different and that just causes developers to wonder why, and waste time investigating.
These tests are not in the patch in #50 but it would be good to have them. At first do it as a separate patch just for the tests, to show it fails, then passes when the code fixes are added.
assertNoField($field)
the comment does make gramatical sense and needs correcting.This was already wrong, not your mistake, but now is a good time to fix it.
That's enough for now. Excellent work so far.
Comment #54
LendudeThanks for the feedback! Fixes for #53
Comment #55
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedLooks like all the review points have been addressed, setting to RTBC
Comment #56
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI am just doing some manual testing on this - I'll report back soon.
Comment #57
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI thought it would be useful, for clarity, to see what happens when the amended tests were run on the existing AssertLegacyTrait.php. I was expecting to see just the one failure for the duplicate buttons. However, what I found was that our additional checks for legacy behaviour with an empty string value only passed for disabled checkboxes. They fail when the checkbox is enabled. This is good becase it means the old sloppy behaviour was only half as bad as first thought ;-)
I also noticed that the new tests for multiple buttons had been added to the textfield tests when they should have been in the non-textfield tests. To simplify the testing, I have divided the 'non-textfield' tests into separate tests for Options, Button and Checkbox types. This makes each test shorter and shows more easily where the failures are. I have also (temporarily) divided the checkbox test into two, so that we can see the second failiure. As I recall on d.o. the test stops at first fail.
The attached patch is expected to fail in three tests -
LegacyFieldAssertsWithButton
,LegacyFieldAssertsWithCheckbox
andLegacyFieldAssertsWithCheckboxTempPartTwo
For accuracy with the existing behaviour we might consider not adding the tests for enabled checkbox with empty string, even though these will pass now (with the updated AssertLegacyTrait code).
Comment #59
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThat's good, it matched what I had observed in local testing. Here's a patch with just changes to the tests which should pass. It does not include the check for duplicate button or the change in the test form, just additional legacy tests using the exitsing
AssertLegacyTrait.php
and these must all remain passing when the real changes are made.Comment #60
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedOK, this is the real complete patch for final review, and interdiff between 54 (Lendude's last patch) and 60. The form and legacyTrait files are not changed since 54. The interdiff is a little hard to read, so here is a summary:
This should be it!
Comment #61
LendudeRefactoring makes sense, lets get this in.
Comment #62
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedYes +1 for RTBC
Comment #63
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedArgH! The status was not meant to change!. That often happens, sorry. The latest comment is displayed after I refresh the browser page, but for some reason the issue status in the form does not change to match the latest status. So when I added a comment it had the effect of reverting the status.
Comment #64
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedYes +1 for RTBC and thanks @jonathan1055 for beeing thorough and taking this much time for the issue!
Comment #65
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedWe had quite a few eyes on the original changes, which got committed fairly promptly because the faults were hindering simpletest conversions. The code was reverted and we have now worked very thoroughly to ensure backwards compatibility and the three of us are all happy with RTBC.
Fundamentally the change is the same as before so there should not be too much (if any) resistance? What is the next step to get this committed?
Comment #66
LendudeA committer needs to find time to look at this. So now we wait, nothing else we need to do.
We have CR so took the tag of.
Comment #67
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedOK.
On the CR I have deleted the second example. That was the unintended and unwanted change which is now reverted out. The CR just covers the primary reason for creating this issue in the first place.
Comment #69
LendudeKnown random fail, #2898721: Random segfault currently in FileFieldWidgetTest::testMultiValuedWidget(), back to RTBC
Comment #71
larowlannit > should these use === because we know the type?
bonus points for more Rabbit
Comment #74
larowlanCommitted as 4c3c1f1 and pushed to 8.5.x.
Cherry picked as 1c5fff9 and pushed to 8.4.x
Comment #75
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThanks larowlan, it's good to have this in.
Just to be clear for others, you say 'cherry picked' but I have just checked and verified that the changes are identical in 8.5 and 8.4.
Comment #76
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedI hope this will help with decreasing the amount of work for the rest of the conversions.
Thanks for the commit and helping me achieve the most important goal this year: getting my own animal into core! This could be the first of many Rabbits to come :)
Comment #77
LendudeWe broke textarea's that contain \n, see #2867154-33: Form: Convert system functional tests to phpunit, not sure if this needs a revert or if we can handle this in a follow up.
Comment #80
larowlanReverted this in light of #2867154-33: Form: Convert system functional tests to phpunit
Comment #81
LendudeHere we go, the should_fail is the original patch with the additional test coverage.
Credit should go to @vaplas for finding and figuring this out.
Comment #82
Lendudeinterdiff was acting a bit wonky, but here we go.
Comment #83
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedYes, I downloaded and compared the patch with #60 to see what you had done.
This all shows that test coverage for the legacy assertions is not comprehensive, but then we knew that anyway right at the start of this issue and the previous ones. But at least now that more tests are being converted the coverage will improve :-)
Comment #84
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedDid not intend to change the status. Something is funny with form values. Back to 'needs review'
Comment #87
LendudeOk that didn't work for elements with no value. Lets see what this does.
Comment #88
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedYes, better to limit the change just to fix just the error we have discovered. Looks fine.
Nice to see that plenty of other modules are now giving us test coverage :-)
You may also want to fix the coding standard in core/modules/system/tests/modules/test_page_test/src/Form/TestForm.php
line 79 Expected 1 space after "="; 0 found
Comment #89
Lendude@jonathan1055 thanks for the feedback!
nit fixed.
Comment #90
Anonymous (not verified) CreditAttribution: Anonymous commentedSorry that I did not take care of this out (time is playing against me today). Thanks @Lendude, for operational work on it!
Comment #91
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThanks for the coding standards correction. Now that core is showing these messages it is good to keep the code clean, especially with #2571965: [meta] Fix PHP coding standards in core progressing well.
To make certain that this fix to AssertLegacyTrait does not cause any other problems with #2867154: Form: Convert system functional tests to phpunit I have tested it with the patch from #31 of that issue. All functional tests pass locally on 8.5, so here is a patch to check on d.o. Obviously this is just for testing - the patch in #89 above is the one we want to commit ;-)
Comment #92
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedNow we know this fix does not cause any other problems, this can be RTBC again.
Comment #93
Lendude@jonathan1055 thanks for the extra testing
Reupping the patch that is RTBC to make sure the last patch in the issue is the one that needs to be committed.
Comment #94
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedIs that the best practice that needs to be done? I will note that for future - I suppose it could cause confusion for a core committer who has not necessarily been involved in every twist and turn of the issue. If so, does the filename need to be changed to match the latest comment number?
Comment #95
Lendude@jonathan1055 yes, the testbot does a daily run on the last patch in any RTBC'd issue, so this is to make sure it is run on the right patch. And yes it also helps the committers figure things out quicker which is patch is RTBC which is also nice.
I never change the file name just as an indication that its just a reup and people shouldn't expect an interdiff or anything, but that might just be me.
Comment #96
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedExcellent. Thanks for the explanation, I should have known that.
Yes, I see it is better not to change the filename if the content of the patch has not changed.
Comment #99
larowlanCommitted as 3609947 and pushed to 8.5.x
Cherry-picked as d9b8afa and pushed to 8.4.x
Thanks for the do-over.
Appreciate the combined patch to @jonathan1055, heading to review that next
Comment #100
larowlanComment #101
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThank you again, larowlan. Sorry we caused you work to revert the commit.
Now that #2867154: Form: Convert system functional tests to phpunit is also in, I think if anyone finds a new discrepancy between Simpletest and AssertLegacyTrait during the many conversions that are happening, it should be raised as a new issue, with a comment added to this thread, so that Lendude and I are notified and can work on the fix (we know this part of the code fairly well by now).