Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
This extract changes from #2747167: Convert first part of web tests of views_ui into its own patch.
Proposed resolution
There are many more, so let's not spend too much time here.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#20 | additional_bc-2750941-20.patch | 24.38 KB | jibran |
Comments
Comment #2
dawehnerComment #3
klausiYippee, great work!
that does not really match. Did you mean fieldNotExists() and fieldValueNotEquals()?
The $path prefixing is not necessary anymore, this is handled by our cleanUrl() override.
I think we should not support that. People should really pass in TRUE/FALSE. Let's add a @todo to remove this and fix all the callers that pass a wrong integer later.
I really hate this two type anti pattern. We should only allow strings in the future. The caller is responsible to make it a string. In most tests it makes zero sense to call t() for asserting a link label. We should really clean up our test cases to never call t(), which is almost always wrong. That is also the reason why we have to mess up our method signatures everywhere with MarkupInterface. For this conversion effort I guess there is not much we can do about it. Sad klausi is sad.
this can be shortened to just $this->assert(empty($links), $message);
same here.
I have the feeling that this method is completely redundant and we just haven't found the mink equivalent yet :) Fine to keep it in the meantime, but let's keep looking.
Comment #4
dawehnerThank you for your quick review klausi!
Oh yeah, totally.
Good point.
Well its 1/0 in the HTML representation.
I totally agree. I'm a bit sad about klausi being sad :)
Oh nice!
Comment #5
klausiComment is wrong. Should be "... is NOT found".
"LinkByHref() should start lower case.
Over 80 characters. I think we can just remove the "fail otherwise" part.
Let's not start with the (string) casting crap in our precious WebAssert class. We should do that in the legacy trait, but all future code should pass only strings because that is more correct.
Hm, you are not casting to string anywhere here, so the comment is wrong?
same here?
and the casting again that I don't like. Can we get rid of it or does that cause a lot of fails?
Comment #6
dawehnerThank you for your great review klausi.
Well, let's try it out
Comment #7
dawehnerComment #8
klausiGood progress, let's make sure we keep our WebAssert class clean of the MarkupInterface conversion nightmare:
there is no optional link index parameter? I think that line should be removed.
If the method also accepts Url then $path must be converted before passing on to WebAssert.
let's keep our WebAssert class clean and only accept string here.
Why is that override needed if it only calls the parent method? Please add a comment.
no, we are not casting anything to string here, so that comment should be removed.
Comment does not match what we are doing here. I think we should remove all the is_string() checking in this method. Just assume a string is passed in, if something goes wrong it is the callers fault.
Comment #9
dawehnerSure, let's fix linkExists as well.
There is no reason :)
Comment #10
klausiI think we should do that on the legacy trait. Our WebAssert is an extension of Mink's WebAssert, so I think we should conform to the data types of the parent class and only allow string. Same case as with MarkupInterface, I think we should not pollute our WebAssert class with this dual parameter type limbo.
If we think about the use case of addressEquals() (calling cleanUrl() down the line) then 95% of use cases will be to just call it with a string. Make sure I'm on the /node/1 path for example. Using Url is a special case and the caller should convert it to string.
Comment #11
dawehnerThe one thing I'm not sure about is the url. The common case/good approach is to use url objects maybe? We should make it not too hard to use that. Its hard to decided in some sane way.
Comment #12
klausiI don't think the common case is to use Url, this looks weird:
which is just bad test code. This would be the right way to do it:
A clear assertion that is a black box test of the actual test outcome - no Url abstraction layer testing in between.
Anyway, this is a philosophical detail we can continue to fight about another time. We should not hold up this issue because of that, otherwise looks RTBC to me.
Comment #14
jibranWe can do this correctly after next line.
Comment #15
klausiGood idea, did a reroll and implemented that.
Comment #16
dawehnerThank you @jibran. That's indeed much better
Comment #17
klausiJust found out in #2737805: Convert web tests to browser tests for forum module that we cannot just remove the buildXPathQuery() method because it used by legacy tests. Should have a wrapper on AssertLegacyTrait.
Comment #18
klausiMerged in requirements from forum test conversion.
Comment #19
dawehnerLet's ensure to not great crazy, but those additions are great!
Comment #20
jibranMoved following change from #2753179: Convert all color web tests to BrowserTestBase here..
Comment #21
dawehnerWell, this is sort of a break at this time, isn't it?
Comment #22
klausiWe introduced BlockCreationTrait only 2 weeks ago to BrowserTestBase, so I think it is fine to change the method name, which will help us a lot for conversions.
This is still RTBC IMO.
Comment #23
dawehnerFair. Let's PLEASE stop adding stuff now.
Comment #24
klausiAgreed, let's freeze this issue on the current changes and do anything else that might come up in follow-ups.
Comment #25
naveenvalecha#23 +1 agreed Let's keep adding stuff in BrowserTestBase.php I have introduced couple of assertion method in contact module conversion. https://www.drupal.org/node/2742995#comment-11344165
Comment #27
catchCommitted/pushed to 8.2.x, thanks!
Comment #28
dawehnerThank you catch! Do you think this could be committed to 8.1.x as well? Many of the other conversions got committed as well.
Comment #29
catchYep.
Comment #30
dawehnercatch commit faster than his shadow!
Comment #32
klausiThanks catch!
Changing this to active for the next round of missing legacy assert helpers, they need to be extracted from #2756059: Convert web tests to browser tests for language module for example.
Comment #33
klausiWrong core version, sorry.
Comment #34
catchLet's please open a new issue for any new additions, otherwise the history gets confusing.
Comment #35
pfrenssenThis should be a protected method, like the other methods in this class. This is causing fatal errors in the tests for Organic Groups. Her is an example from build 140793354:
I'll make a followup to fix this.
Comment #36
pfrenssenCreated followup: #2757139: Fix visibility of AssertLegacyTrait::buildXPathQuery().
Comment #37
klausiWe will continue in #2759879: Additional assertions for WebAssert and AssertLegacyTrait, part 2.
Comment #39
klausi