Problem/Motivation

For #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) we need more assertion helper methods to provide a richer page elements assertion experience for our PHPUnit browser tests and to make converting exiting web tests easier. The first part was done in #2750941: Additional BC assertions from WebTestBase to BrowserTestBase.

Proposed resolution

Add assertion methods as we find and need them.

Remaining tasks

Patch.

User interface changes

None.

API changes

Only API additions.

Data model changes

None.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

klausi created an issue. See original summary.

klausi’s picture

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -129,6 +129,27 @@ protected function assertFieldByName($name, $value = NULL) {
    +   * Asserts that a field exists with the given name and value.
    +   *
    +   * @param string $name
    +   *   Name of field to assert.
    +   * @param string $value
    +   *   (optional) Value of the field to assert. You may pass in NULL (default)
    +   *   to skip checking the actual value, while still checking that the field
    +   *   exists.
    +   *
    +   * @deprecated Scheduled for removal in Drupal 9.0.0.
    +   *   Use $this->assertSession()->fieldNotExists() or
    +   *   $this->assertSession()->fieldValueNotEquals() instead.
    +   */
    +  protected function assertNoFieldByName($name, $value = NULL) {
    

    Oops, doc block is wrong. This should be "Asserts that a field does NOT exist ..."

  2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -344,6 +365,47 @@ protected function assertNoOption($id, $option) {
    +   * Asserts that a select option in the current page is checked.
    +   *
    +   * @param string $id
    +   *   ID of select field to assert.
    +   * @param string $option
    +   *   Option to assert.
    +   *
    +   * @deprecated Scheduled for removal in Drupal 9.0.0.
    +   *   Use $this->assertSession()->optionSelected() instead.
    +   */
    +  protected function assertOptionSelected($id, $option) {
    

    Should probably be "is selected" instead of "is checked".

jibran’s picture

dawehner’s picture

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -191,6 +191,29 @@ public function optionNotExists($select, $option, TraversableElement $container
   /**
+   * Checks that a specific option in a select field is selected.
+   *
+   * @param string $select
+   *   One of id|name|label|value for the select field.
+   * @param string $option
+   *   The option value.
+   * @param \Behat\Mink\Element\TraversableElement $container
+   *   (optional) The document to check against. Defaults to the current page.
+   *
+   * @return \Behat\Mink\Element\NodeElement
+   *   The matching option element
+   *
+   * @throws \Behat\Mink\Exception\ElementNotFoundException
+   *   When the element doesn't exist.
+   */
+  public function optionSelected($select, $option, TraversableElement $container = NULL) {
+    $option_field = $this->optionExists($select, $option, $container);
+    $this->assert(!empty($option_field->getAttribute('selected')), sprintf('Option "%s" in select "%s" is not selected as expected.', $option, $select));
+
+    return $option_field;
+  }
+

I was wondering whether we should somehow group all those form related assertions in its own trait. I'm also not sure whether we really should just add thousand of assertions again.

klausi’s picture

Yes, optionSelected() is a bit of an edge case, you can also do this directly in your test:

$option_field = $this->optionExists('Content type', 'article');
$this->assertNotEmpty($option_field->getAttribute('selected'));

It makes it harder to read than

$this->assertSession()->optionSelected('Content type', 'article');

So I'm in favor of keeping it as convenience function to make tests more readable.

dawehner’s picture

While its harder to read its more explicit.

@lendude had some opinions about not introducing all assertions just for the sake of it.

Lendude’s picture

well @dawehner asked for it, so here comes my totally unfounded opinion:

I'm all for making tests readable, but to my mind

$this->assertNotEmpty($this->optionExists('Content type', 'article')->getAttribute('selected'));
or
$this->assertTrue($this->optionExists('Content type', 'article')->hasAttribute('selected'));

is perfectly readable. If you can't figure out what that does, you have no business messing with that code. Readability is fine, but we don't have to make it so our junior account manager has to be able to understand what's going on (there are occasions where that's useful, testing Drupal core isn't one of them) . And if you think the test is hard to figure out for the next person that reads it, add a comment, like you do for the rest of your code.

More assertions means more code to maintain, more possible BC issues when we move on to the next hot testing framework and more assertions to get lost in when trying to write tests (seriously, I never know which one is the "right" one).

Stuff like assertWaitOnAjaxRequest that @jibran added in #2648956: Editing a view leaves old keys hanging, results in invalid schema I think is awesome, it standardizes something that we already saw 3 different methods for in 4 javascript tests. I feel that is the sort of stuff we need to add, the rest is just bloat.

To my way of thinking there are two ways to go, really lean (not a lot of asserts for readability sake), or really readable (lots of well documented asserts). I'm just afraid that we will wind up somewhere in the middle of we go for readable. So why not just go lean.

/rant off

klausi’s picture

Fine with me, we can always add this method later when we want to go down the readability route :)

The important part of this patch are the additions to AssertLegacyTrait that we need to ease PHPUnit conversions.

klausi’s picture

Status: Needs work » Needs review
FileSize
5.94 KB
5.49 KB

Added assertPattern() and assertUniqueText() for legacy support. Removed the changes from WebAssert.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Cool. Let's continue in yet another issue :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: asserts-part2-2759879-10.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
7.15 KB
5.04 KB

If we decided that we keep messages in test conversions then we need to keep also the message argument in assertion methods signatures.

klausi’s picture

  1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -94,7 +94,7 @@ protected function assertNoText($text) {
    -   * Passes if the text is found ONLY ONCE on the text version of the page.
    +   * Passes if the text is found only once on the text version of the page.
    

    let's leave it all caps because that is the important part.

  2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -115,12 +115,12 @@ protected function assertUniqueText($text, $message = NULL) {
    -    $nr_found = substr_count($page_text, $text);
    -    $this->assertSame(1, $nr_found, $message);
    +    $occurrences = substr_count($page_text, $text);
    +    $this->assertSame(1, $occurrences, $message);
    

    hm, I don't think changing this variable name is necessary? $nr_found is pretty clear?

  3. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -189,12 +189,17 @@ protected function assertFieldByName($name, $value = NULL) {
    -  protected function assertNoFieldByName($name, $value = NULL) {
    +  protected function assertNoFieldByName($name, $value = NULL, $message = '') {
    

    $message is unused in the function body, so it never makes sense to pass it in? I think we should just leave it out. PHP is robust enough to just ignore additional method parameters.

So I think we should go with the patch from #10 instead, hopefully it was a random failure anyway.

claudiu.cristea’s picture

@klausi

#14.2:

hm, I don't think changing this variable name is necessary? $nr_found is pretty clear?

The reason I changed that is because the "nr" abbreviation doesn't sound to me too English. I might be wrong, I'm not a native English speaker. "Nr" is OK in my mother tongue (Romanian) and also in other languages like German but I think the correct abbreviation in English is "No". But I have no strong opinion, OK with the variable name.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Strictly speaking you can pass along additional variables. PHP will happy ignore them. You can even do the opposite and just get a warning ... anyway.

let's leave it all caps because that is the important part.

Well you know, I don't care much. Let's just go with the thing in the patch. You know, this is not about perfectionism here. We can always iterate later.

  • catch committed 4bc3405 on 8.2.x
    Issue #2759879 by klausi, claudiu.cristea, dawehner, Lendude, jibran:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4bc3405 and pushed #10 to 8.2.x. Thanks!

dawehner’s picture

Version: 8.2.x-dev » 8.1.x-dev

@catch
Can we put that into 8.1.x as well?

dawehner’s picture

Status: Fixed » Reviewed & tested by the community

  • catch committed 5c8dd62 on 8.2.x
    Revert "Issue #2759879 by klausi, claudiu.cristea, dawehner, Lendude,...
catch’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Reviewed & tested by the community » Needs work

xjm pointed out it'd be much better if simpletest AssertContentTrait called the new methods instead of the code being duplicated, and that makes sense to me as well. Have reverted for now.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

I'm sorry but this doesn't make sense for me

AssertContentTrait is for simpletest, so it looks like

  protected function assertFieldChecked($id, $message = '', $group = 'Browser') {
    $elements = $this->xpath('//input[@id=:id]', array(':id' => $id));
    return $this->assertTrue(isset($elements[0]) && !empty($elements[0]['checked']), $message ? $message : SafeMarkup::format('Checkbox field @id is checked.', array('@id' => $id)), $group);
  }

The path adds

+  protected function assertFieldChecked($id, $message = '') {
+    $this->assertSession()->checkboxChecked($id);
+  }

which is defined in \Behat\Mink\WebAssert::checkboxChecked.

Where exactly is this code duplication?

Maybe here?

+  protected function assertUniqueText($text, $message = NULL) {
+    // Cast MarkupInterface objects to string.
+    $text = (string) $text;
+
+    $message = $message ?: "'$text' found only once on the page";
+    $page_text = $this->getSession()->getPage()->getText();
+    $occurrences = substr_count($page_text, $text);
+    $this->assertSame(1, $occurrences, $message);
+  }

Not really IMHO, its total different code:

  protected function assertUniqueTextHelper($text, $message = '', $group = 'Other', $be_unique = FALSE) {
    // Cast MarkupInterface objects to string.
    $text = (string) $text;
    if (!$message) {
      $message = '"' . $text . '"' . ($be_unique ? ' found only once' : ' found more than once');
    }
    $first_occurrence = strpos($this->getTextContent(), $text);
    if ($first_occurrence === FALSE) {
      return $this->assert(FALSE, $message, $group);
    }
    $offset = $first_occurrence + strlen($text);
    $second_occurrence = strpos($this->getTextContent(), $text, $offset);
    return $this->assert($be_unique == ($second_occurrence === FALSE), $message, $group);
  }
klausi’s picture

Agreed with dawehner - we cannot use AssertContentTrait because we must use the Mink API, which only exists on BrowserTestBase and cannot be shared with Simpletest.

xjm’s picture

xjm’s picture

So my concern is that we are adding untested code that is deprecated at the time we add it (and making decisions about the future API in the process). I definitely agree it makes sense to add shivs that make the conversions easy, but I am concerned about adding untested assertions, even to a deprecated shiv layer (in fact almost especially then). Code that does the same thing but is written differently is risky too.

Is there any way we could at least jerry-rig something that tests both code paths for the same method in different test suites?

xjm’s picture

Or in another issue, there were some creative use statements.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Thinking about it, I also disagree with @Lendude and think that if we are adding a BC shiv, it should be universal, so that we can convert tests at scale rather than having to rewrite them every time a method is missing. Whether we retain the methods after the conversion can be sorted in #2770549: [plan] Discuss desired API and test suite following deprecation of SimpleTest etc. To that end, should we also consider adding them as @internal? Or what is best? SimpleTest's public API needs to stick around until 9.0.0, but we can gut it or change the internals (if we do so with tests etc. in a safe way).

dawehner’s picture

Well the legacy trait is deprecated already ... that sounds like the right indication for people here ... don't use that, unless your are lazy.

I agree with lendude in general, often just adding a shit ton of assertions helpers doesn't help that much. ON the other hand though, assertUnique seems not the worst idea to be honest to have in the first place. All the other ones are simple BC wrappers, so they should be fine.

Given that, should we add the assertUnique one onto WebAssert itself?

xjm’s picture

Given that, should we add the assertUnique one onto WebAssert itself?

That could help, yeah -- especially with a little unit test to make sure it works. If we want to deprecate it later we still can.

Looking at #2750941: Additional BC assertions from WebTestBase to BrowserTestBase, most of the methods in the legacy trait are actually just one-line wrappers for phpunit API, which feels much safer and doesn't really need its own test coverage. And the methods that take a few lines of logic are on WebAssert. (Though not with test coverage even for those -- maybe that is a separate followup for that?)

For a couple minors probably, this shiv actually is going to be the API in practice. We definitely don't want to make these things wrappers for SimpleTest methods -- the SimpleTest methods would be the ones that should be wrappers if anything. Maybe that is not possible, but I feel like there might be some solution that won't require us to maintain two sets of deprecated code to do the same thing for different testing APIs. If it isn't possible, then let's add things with basic test coverage.

I think ideally the goal should be that we want to be able to post one single test patch that adds/changes some use statements for all web tests in core, moves them, and changes which class they extend... and then we see what tests fail and that tells us what still needs work in our new API. And all the lines that are $this->assertWhatever() do not need to change at all at least in the initial conversion. Then we hack it up into scoped issues by concept rather than test, peeling off blockers when that doesn't work with some method (or alternately decide that it's not feasible to do it that way yet).

And then in a later phase we might have issues like "assertRobot() is not a useful method in the testing API; deprecate it". But not until we've validated that we can do the first part of the conversion with what we have.

Is there a list of web test methods anywhere that exist in vs. are not available in the new API? Like: web test assertion, and what it's covered by (if anything) for our new API, a legacy trait assertion or WebAssert or whatnot. (We could even put that in the change record so contrib authors can see at a glance how to write future-proof tests.)

Sorry for stumbling in with these questions only now; somehow it didn't occur to me when I looked over #2750941: Additional BC assertions from WebTestBase to BrowserTestBase.

dawehner’s picture

I had this gigantic comment, but I felt like being an asshole, so I have stored it locally to not forget some of the points, but I hope this comment is more constructive.

(Though not with test coverage even for those -- maybe that is a separate followup for that?)

Alright, let's open up a follow up. That's easy: #2770907: Add test coverage for WebAssert

Maybe that is not possible, but I feel like there might be some solution that won't require us to maintain two sets of deprecated code to do the same thing for different testing APIs. If it isn't possible, then let's add things with basic test coverage.

There might be, but I think the maintenance cost of two similar codebases is reasonable vs. coming up with another abstraction layer in the middle. This abstraction layer would make it harder for people actually using simpletest to understand what is going on, which is a bigger group of people, than people maintaining the test suite. I think we both totally agree on the point that BrowserTestBase should be the way to go and make that as best as possible.

I think ideally the goal should be that we want to be able to post one single test patch that adds/changes some use statements for all web tests in core, moves them, and changes which class they extend... and then we see what tests fail and that tells us what still needs work in our new API.

Well, we kinda do that at the moment, but not as a gigantic patch, because that's just an insane amount but rather on a module per module base. This gives us also the advantage to see assertions popping up over time.

Is there a list of web test methods anywhere that exist in vs. are not available in the new API? Like: web test assertion, and what it's covered by (if anything) for our new API, a legacy trait assertion or WebAssert or whatnot. (We could even put that in the change record so contrib authors can see at a glance how to write future-proof tests.)

Well, that is the thing we are finding out by trying out these conversions. I would make the point that we don't have the experience about how our test suite looks like after 9-10 years of organic growth.

@xjm
Can you please elaborate your idea, maybe in the context of the current work:

  1. What is wrong with the current approach which works: We convert things and find out what we need to introduce by doing so. We are doing the research work already, but just not upfront, but as we go. More for that under [1]
  2. What is your proposed alternative way of doing things? Please read [1] maybe first

[1]
The current way of doing things, and which actually works, as we have already a lot of tests converted and standardized on a set of additional assertions already:

  • Convert a module, or a subset of module tests (see the views_ui issue)
  • Find out which assertions a) don't exist yet b) should be an actual assertion c) should be just a BC layer
  • Continue with
dawehner’s picture

Let's also actually execute this use statement idea: #2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits
While I think its a great idea, thank you @xjm, I don't believe that this should be a requirement upfront, but rather it could be a parallel effort.

klausi’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Unit tests for our WebAssert: yes, ideally we should have them. Then again it feels a bit pointless when the method is used by many test cases and is tested implicitly a lot already. But yes, dawehner opened the issue, let's do it.

Conversions: I agree that they should be as minimal as possible for the test class and we should aim for maximum legacy API compatibility, so that people have an easier time converting their tests.

Otherwise I agree with dawehner: we should do the iterative approach with small steps we are currently doing. That way tests are converted already, so people get familiar gradually, the testbot gets familiar gradually ... we smooth the transition in instead of doing one big bang.

So let's get back to some actionable items for this issue:
whether or not assertUniqueText() should be part of our WebAssert is a topic for #2770549: [plan] Discuss desired API and test suite following deprecation of SimpleTest. In the meantime we do the thing that we all agree on: we need assertUniqueText() on the legacy trait in any case. Currently the logic lives there, so we need test coverage for it. So when we have test coverage for that, this issue can go back to RTBC, right?

klausi’s picture

Assigned: Unassigned » klausi

working on tests.

klausi’s picture

Assigned: klausi » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
10.9 KB
4.96 KB

I added the unit tests for the legacy trait in the namespace Drupal\Tests\Core\Assert.

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/Assert/AssertLegacyTraitTest.php
@@ -0,0 +1,173 @@
+/**
+ * @coversDefaultClass \Drupal\FunctionalTests\AssertLegacyTrait
+ * @group Assert
+ */
+class AssertLegacyTraitTest extends UnitTestCase {

It is a bit weird that we don't move this assertion into webassert but rather keep it as part of the legacy trait. Are we sure about that?

klausi’s picture

Yes, since we are not sure yet whether we want it on webassert or not. If we move it to webassert later then we can also move the tests.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

That's a fair point :)

To be honest I kinda would have used a data provider for those quite similar test cases, but I guess this is totally up for people to decide

claudiu.cristea’s picture

+1 for RTBC. Let's push this in as it block all other conversions.

xjm’s picture

Having test coverage makes me more comfortable with this. I'm still somewhat hesitant, but as @dawehner points out, having some kind of magical BC layer instead would have its own risks. I certainly don't want to block ongoing work, since one way or the other we are already maintaining multiple APIs, and this issue only slightly increases the surface area of one.

I looked at the other issue I was thinking of, #2758067: BrowserTestBase::drupalCreateUser() should use UserCreationTrait::createUser(), and realized the strategy used there isn't applicable here. That adds API wrappers for actions taken on the child site -- whereas this issue adds API for the test runner itself. Similarly, while #2759853: Create ConfigTestTrait to share code between PHPUnit and Simpletest is related to the setup of the child site for both test runners, that's still Drupal API functionality rather than test runner API functionality.

I looked over the added tests closely, and it adds coverage for different code paths for the methods that are not just pure wrappers. +1

That way tests are converted already, so people get familiar gradually, the testbot gets familiar gradually ... we smooth the transition in instead of doing one big bang.

See https://www.drupal.org/core/scope for why this often doesn't actually work that way in practice. Out of scope for this issue though. ;)

Committed and pushed to 8.2.x. Thanks!

  • xjm committed df1fe18 on 8.2.x
    Issue #2759879 by klausi, claudiu.cristea, dawehner, xjm, Lendude,...
Eric_A’s picture

Why wasn't this opened and applied against the stable branch?

Now bug fixes in the testing framework are forced to be forked. One example here: #2773733-12: Fix AssertLegacyTrait::assert(No)Text to handle html encoded strings (part 3).

dawehner’s picture

Version: 8.2.x-dev » 8.1.x-dev
Status: Fixed » Needs review

Sure, let's try to get it in.

Eric_A’s picture

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

I ran the 8.1.x tests against the patch from #35 and it passes. RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 35: webassert-2759879-35.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community

Yes, patch fail to apply on 8.2.x but still valid for 8.1.x.

Eric_A’s picture

That's just the patch failing to apply to 8.2.x. (It's in already). RTBC per #45.

xjm’s picture

@Eric_A, the issue is an API addition, which in general are only allowed against the development minor. See https://www.drupal.org/core/d8-allowed-changes and https://www.drupal.org/core/d8-bc-policy. Tests themselves are considered internal, but the testing framework is expected to be public API so contrib authors, sites with tested custom modules, deployment workflows, etc. can rely on them. However it is a tricky situation which is why I posted #2773087: [policy, maybe patch] Clarify backport policy for testing framework changes, including new BTB tests.

#42 is a good reason to consider backporting these fixes. Thanks for commenting on #2773087: [policy, maybe patch] Clarify backport policy for testing framework changes, including new BTB tests; I'll follow up more there.

dawehner’s picture

@xjm
What about committing it now and then change our behaviour, in case we reach a different conclusion in #2773087: [policy, maybe patch] Clarify backport policy for testing framework changes, including new BTB tests. We have committed those additions to the testing framework since
a while to both 8.2 and 8.1 and then suddenly we stopped doing. By stopping one looses momentum.

  • xjm committed 121857c on 8.1.x
    Issue #2759879 by klausi, claudiu.cristea, dawehner, xjm, Lendude,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Alright, based on git diff 8.2.x -- core/tests/Drupal/Tests/BrowserTestBase.php as well as feedback from @Eric_A, @dawehner, and @Berdir on #2773087: [policy, maybe patch] Clarify backport policy for testing framework changes, including new BTB tests, I cherry-picked this back to 8.1.x. I think we still have things to sort out on that policy for the 8.3.x cycle though so ongoing help there is appreciated.

Thanks everyone!

dawehner’s picture

Thanks a ton @xjm!

  • catch committed 4bc3405 on 8.3.x
    Issue #2759879 by klausi, claudiu.cristea, dawehner, Lendude, jibran:...
  • catch committed 5c8dd62 on 8.3.x
    Revert "Issue #2759879 by klausi, claudiu.cristea, dawehner, Lendude,...
  • xjm committed df1fe18 on 8.3.x
    Issue #2759879 by klausi, claudiu.cristea, dawehner, xjm, Lendude,...

  • catch committed 4bc3405 on 8.3.x
    Issue #2759879 by klausi, claudiu.cristea, dawehner, Lendude, jibran:...
  • catch committed 5c8dd62 on 8.3.x
    Revert "Issue #2759879 by klausi, claudiu.cristea, dawehner, Lendude,...
  • xjm committed df1fe18 on 8.3.x
    Issue #2759879 by klausi, claudiu.cristea, dawehner, xjm, Lendude,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.