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.
Comment | File | Size | Author |
---|---|---|---|
#25 | interdiff.txt | 1.36 KB | xjm |
#25 | 2469731-25.patch | 1.23 KB | xjm |
#14 | interdiff.txt | 676 bytes | pfrenssen |
#14 | document-btb-2469731.14.patch | 1.15 KB | pfrenssen |
#12 | interdiff.txt | 1.06 KB | larowlan |
Comments
Comment #1
larowlansure
Comment #2
klausiNo security vulnerability, unusable system, data loss or automated test fails, so not critical.
Comment #3
chx CreditAttribution: chx commentedExcept 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?
Comment #4
larowlanFYI https://www.drupal.org/contribute/core/beta-changes#unfrozen includes 'Automated tests'
Comment #5
chx CreditAttribution: chx commentedI 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:
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.
Comment #6
hussainwebJust 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.
Comment #7
larowlanActually, its not for JavaScript until we add step 2.
So maybe we should just say 'contrib' testing for now?
Comment #8
chx CreditAttribution: chx commentedWhat about discouraging use *at all* until Step 2?
Comment #9
hussainwebI 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.
Comment #10
clemens.tolboomComment #11
pfrenssenThe 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.
Comment #12
larowlanDon'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.
Comment #13
larowlanComment #14
pfrenssenLooks 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.Comment #15
hussainwebYes, agreed. Let's get this in. RTBC +1.
Comment #16
dawehner... So did anyone considered to add those bits into the BrowerTestBase class?
Comment #17
xjmComment #18
xjmI 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.
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).
Comment #19
xjmComment #20
chx CreditAttribution: chx commentedI 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:
and compare to the first two two comments after commit and congratulations:
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.
Comment #21
nod_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)
Comment #22
Fabianx CreditAttribution: Fabianx for Acquia commented#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.
Comment #23
xjmWe 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.
Comment #24
xjmUpdating the patch.
Comment #25
xjmI 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.
Comment #26
chx CreditAttribution: chx commentedThis does not address what contrib authors should or should not do.
Comment #27
xjm@chx, I think this is pretty clear:
...followed by a list of specific issues that need to be addressed.
Comment #28
chx CreditAttribution: chx commentedOK. I'll leave to those who actually worked on this to RTBC it.
Comment #29
pfrenssenOK, thanks for clearing it up.
Comment #30
alexpottCommitted 5af2cf6 and pushed to 8.0.x. Thanks!
Comment #32
dawehnerAfter some discussion with chx and fabianx we updated the issue summary in https://www.drupal.org/node/2469723/revisions/view/8346999/8381525