Additional assertions for WebAssert and AssertLegacyTrait, part 2

Let's add assertPattern() from #2753179: Convert all color web tests to BrowserTestBase.

+++ 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.

While its harder to read its more explicit.

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

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

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

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

@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.

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.

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

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

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.

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);
  }

Posted #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) also as a separate discussion.

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?

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

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).

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?

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.

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

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.

+++ 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?

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

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

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!

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).

Sure, let's try to get it in.

Sure, let's try to get it in.

git cherry-pick df1fe18

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

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

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

@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.

@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.

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!

Thanks a ton @xjm!