Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

sure

klausi’s picture

Priority: Critical » Major

No security vulnerability, unusable system, data loss or automated test fails, so not critical.

chx’s picture

Priority: Major » Critical

Except a hell lot of confusion. Sure it's not HEAD broken, it's just community broken. Since you have already thrown out the processes we have in place by committing a feature to D8.0 is it too much to ask to at least document expeditiously what you are doing?

larowlan’s picture

chx’s picture

I have two problems.

1. It is absolutely inevitable this will divert attention and resources that should go into getting core and contrib out the door ASAP. I am trying to mitigate this by filing this issue to put a quick end to everyone trying to write tests based on the not 100% complete shiny new thing.

2. I have complained a few times in the past about how we stopped doing requirements analysis and choosing the right tool for the right job (like Symfony was choosen for HTTP Foundation -- that was great! Also, sort of happened with Twig where at least quite some reasoning went into how it's better than phptemplate -- there weren't many other candidates.). It's bad enough to have phpunit for unit tests but that's somewhat manageable because those tests, how to say, don't do a lot. The following problems make writing and debugging phpunit tests really hard:

  1. It is not using native namespacing which makes it different from core and contrib. Not that big of a deal but it's one among the many.
  2. The asserts are very abstracted. This makes it much harder to debug problems. Like, try to follow something as simple as assertNotNull. You will travel through the Assert, PHPUnit_Framework_Constraint_Not and PHPUnit_Framework_Constraint_IsNull classes for what is essentially a !== NULL.
  3. with() is not constraining, it only asserts. This is very confusing when trying to return different things to two invocations discernible only by different arguments (yes, yes, use a callback, I know, that's not the point).
  4. The mock objects are put together in memory and eval'd. They should be written to temp and included. This, again, hinders debugging really badly.
  5. The mock object templates are hardwired to the test runner dir which makes it impossible for core to extend them. I wanted to because there's no "strict mode" where any calls not specifically expected to a mock object triggers a failure (it is not impossible this feature is available under some name I am unaware of but I couldn't find it reading the code).

I wish there were some research done whether atoum or testify (or anything else I am unaware of) would've served better. If there was, my apologies.

hussainweb’s picture

FileSize
615 bytes
596 bytes

Just fixing nitpicks with punctuations. Interdiff is mostly useless but attached anyway. I am leaving it to RTBC as it is just a very small change.

larowlan’s picture

+++ b/core/modules/simpletest/src/BrowserTestBase.php
@@ -31,6 +31,10 @@
+ * Currently, this class should only be used for JavaScript testing. For

Actually, its not for JavaScript until we add step 2.
So maybe we should just say 'contrib' testing for now?

chx’s picture

What about discouraging use *at all* until Step 2?

hussainweb’s picture

Status: Reviewed & tested by the community » Needs review

I am kind of leaning towards @chx in #8 at the moment. And I also agree with @larowlan in #7. While I love using Behat and Mink, in it's current form, it is going to create confusion. There is no clear advantage to moving in one direction or the other (until step 2 at least) and there is a small chance that things might change when we add in step 2.

Don't get me wrong. It is great that this is in, but until there are bigger use cases, it could be a breeding ground for confusion and discouraging it for now does make it easier to handle that. Considering the time it took to get the step 1 in, it is better to document it so until we can build a more solid use case.

Since this is a discussion point, I think we should set this to "Needs Review" until we hear more thoughts.

pfrenssen’s picture

The change record for the BrowserTestBase issue is a good starting point for this: New base testing class added to core to facilitate longer term modernization of Simpletest. It already explicitly mentions that this class is not to be used for Drupal core until 8.1.

Probably we don't need to much information here. Maybe just mentioning that it is only to be used in contrib, and referring to the change record would be enough.

larowlan’s picture

Don't agree that this is critical but don't care enough to argue.
Reworded as per #8, #9 and added some notes from the change-notice.

Can we get a quick turnaround on review/rtbc here - the intent of this issue - and the reason I assume chx made it critical - is to ensure no-one starts writing core tests with it just yet.

larowlan’s picture

pfrenssen’s picture

FileSize
1.15 KB
676 bytes

Looks good, just a minor thing: this documentation is added to the top of the docblock before the @ingroup tag, and the remainder of the documentation is behind the tag. This makes the two parts of documentation look divorced from eachother.

hussainweb’s picture

Yes, agreed. Let's get this in. RTBC +1.

dawehner’s picture

+++ b/core/modules/simpletest/src/BrowserTestBase.php
@@ -31,12 +31,25 @@
  * All BrowserTestBase tests must have two annotations to ensure process
  * isolation:
  * - @runTestsInSeparateProcesses
  * - @preserveGlobalState disabled

... So did anyone considered to add those bits into the BrowerTestBase class?

xjm’s picture

Priority: Critical » Major
xjm’s picture

Category: Bug report » Task
  1. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -31,12 +31,25 @@
    + * Currently, this class should only be used for contrib module testing. Note
    + * that it does not have feature parity with WebTestBase and is notably missing
    + * the following features:
    

    I don't think the first sentence here really has a place in our API documentation; what we have in the change record should be sufficient.

  2. +++ b/core/modules/simpletest/src/BrowserTestBase.php
    @@ -31,12 +31,25 @@
    + * Use of this module for tests in core is postponed until 8.1.x.
    

    In #2232861-188: Create BrowserTestBase for web-testing on top of Mink Dries asked that we not start converting existing tests, but just use it for testing things that cannot currently be tested with SimpleTest (once that is possible). I think we can just remove this sentence and manage this in our issue queue (marking issues to convert existing test coverage postponed).

xjm’s picture

Status: Reviewed & tested by the community » Needs work
chx’s picture

I understand this does not fit the definition of critical however look at Dries' comment #2232861-188: Create BrowserTestBase for web-testing on top of Mink about the disruption this will cause:

people will have to learn about this tool, people will be inclined to rewrite existing tests

and compare to the first two two comments after commit and congratulations:

  1. we need a big big meta issue to convert all the WebTest to BrowserTest.
  2. it's left a bit unclear now what this means for people writing functional tests.

by jibran and fago, no less. In my humble opinion we need to do more than a change record to stop articles appearing etc (the first half of Dries' comment: people will have to learn about this tool). I tried to articulate how this affects core and contrib both in #5 but perhaps I was less successful in communicating as that comment went completely unanswered and #17 demoted this without a word.

Another problem I have is that the technical concerns raised in #5 also went unanswered. In case this is not the right issue I have linked it from #2232271: [Meta] Use Behat for validation testing but it did not get replies there either. It is the only relevant meta I could find. What is the right place to raise these concerns?

Now some meta: I have rewritten this comment a number of times trying to chose such words that will not be misunderstood as some sort of attack, to remain strictly factual. I hope I have succeeded, this time. Constructive criticism of this level (how could I write this even better?) is more than welcome. I am trying to establish a new tone, one that's hopefully less controversial and more helpful.

nod_’s picture

During our core conversation The Future of Drupal Functional Testing with Konstantin Kudryashov (from behat, mink and more) he mentioned that we shouldn't port all old simpletest tests to the new thing (link to the recording at the time of that specific question), as a real client project would never do that and only use the new suite for new tests. I don't know what's the status is on that but gradually deprecating makes sense.

I know it doesn't answer all your questions, but it's a start.

(for what it's worth, found the comment clear and non confrontational, was a pleasure to read)

Fabianx’s picture

#18.2: I think this is an exception and people writing JS tests will know how to do so (and that those are allowed). I agree it is important to state this is postponed to 8.1.x. Maybe add: "Javascript tests are the only tests that are allowed in Core as of now."

So I vote for leaving that sentence in for now - maybe slightly changed.

xjm’s picture

We don't document other, much more important release management policies in the codebase, so there is no need to do so in this case just because two individual contributors who worked on the original patch overlooked Dries' original comment. Those two contributors now know.

What we should do instead is clarify in the beta evaluation policy that, while SimpleTest and PHPUnit improvements are unfrozen, BrowserTestBase use should be limited to new coverage. That's the place for core policy during the beta. No need to have one special case buried in the codebase.

xjm’s picture

Assigned: Unassigned » xjm

Updating the patch.

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review
FileSize
1.23 KB
1.36 KB

I updated the beta evaluation policy with this note: https://www.drupal.org/node/2368133/revisions/view/8330953/8353666

Attached patch implements the changes I suggest for the API-level documentation.

chx’s picture

This does not address what contrib authors should or should not do.

xjm’s picture

@chx, I think this is pretty clear:

WebTestBase should be used where possible

...followed by a list of specific issues that need to be addressed.

chx’s picture

OK. I'll leave to those who actually worked on this to RTBC it.

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community

OK, thanks for clearing it up.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5af2cf6 and pushed to 8.0.x. Thanks!

  • alexpott committed 5af2cf6 on
    Issue #2469731 by xjm, larowlan, pfrenssen, hussainweb, chx: Document...
dawehner’s picture

After some discussion with chx and fabianx we updated the issue summary in https://www.drupal.org/node/2469723/revisions/view/8346999/8381525

Status: Fixed » Closed (fixed)

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