Rebased.
Wrong file name, should work now.
Thanks for review, PR updated and ready for the review.
Tests failed need to check.
@drdam please provide change record to this issue.
And test doesn't seem to be updated.
Set back to NW.
Thanks for update MR. New wording looks good, and thanks for provide information about caching.
I check MR again and have one more question.
I love this feature, and I am going to use it as soon it will be deployed. Thanks for your effort.
Thanks, RTBC.
Thanks for MR.
Do you think we can add interface for these new methods?
MR looks good. But should we add test coverage new method at Drupal\Tests\link\Unit\LinkFormatterTest?
Thanks for MR, this feature looks really nice, but as I understand expected change in the UI already change at #77.
Please update IS.
I left my feedback at MR, please check.
I was wonder about theme used, but you already mentioned that there is no theme.
I don't clearly understand what does it mean?
You already mentioned limit - "block placing". Are there any other limits?
Looks like a great idea, thanks.
I left my feedback on MR.
Add internal links.
Improved error message. Provide violation message.
Update IS.
I don't understand why review bot fails on phpstan. MR check looks good.
Add tag to not ignore bot.
@chi this looks like a good suggestion. I believe there are more places where this can be done.
I mad MR 10993. MR 11001 looks like good fix. @godotislate thanks.
+1 for RTBC
Yes, it's very concerning if functional javascript tests have false negatives on MR builds.
I think this false positive. Tests passes but it should fail.
@nicxvan thanks for double check original MR, so do I and also found nothing.
Thanks, in this case LGTM.
Looks like some random fails. Please double check.
MR rebased
LGTM. +1 for RTBC.
Thanks a lot for rebasing and merge it.
Thank you @berdir, @quietone. For the links and the explanations.
Thanks @nicxvan for review, I update MR.
Decide to improve Hook attribute tests 🐛 Improve HookCollectorPass test Active .
Create MR.
Not sure that implement points 2, 3 with changes at
- Drupal\hook_collector_on_behalf\Hook\OnBehalfOfOtherModuleHook
- Drupal\hook_collector_skip_procedural\Hook\SkipProceduralHooks
is perfect way.
Maybe we need to check it at UnitTest "Drupal\Tests\Core\Hook\HookCollectorPassTest", but I don't know how this can be achieved.
nikolay shapovalov → created an issue.
Looking forward to merge this. I really need this for several projects.
Thanks @nicxvan, I remove special handling for "hook_hook_info()".
@klausi thanks for your review, I update PR and it's ready for review.
@nicxvan, I add special handling for hook_hook_info(). But still not sure about it.
Thanks, it's very food question.
Because when combine them together and now use base class, it will be test with more then 500 lines of code.
It's easy to read and understand for me.
But do you have any suggestion?
Create draft MR, need to decide what other internal links to test.
nikolay shapovalov → created an issue.
nikolay shapovalov → created an issue.
Thanks for all you effort, but from my side this doesn't look like bug.
It looks like default behavior.
My suggestion if you want this behavior create custom formatter, or contrib module that implements this.
I revert changes that is not a part of actual scope. You can see commit dc3a17f7.
These can be implemented in follow up to improve test speed for Drupal\Tests\link\Functional\LinkFieldTest
:
-
Improve
::assertValidEntries(), ::assertInValidEntries()
Check multiple values at single request.
Check exact value, instead of whole page. - use
EntityTest::create()
everywhere in test. - do not add field to ViewDisplay, FormDisplay if it's not used.
- use static field name instead of random field, to make easy to debug test
MR is ready for review.
Update IS.
Found way to improve assertInvalidEntries
speed.
But I don't understand why when LinkWidget::validateUriElement
throws error "Manually entered paths should start with one of the following * characters: / ? #" all other errors are hidden. Other errors provided not by element validation, but with constraint.
I would say this is bad UX, because for example you submit several invalid links, and at first submission you see only one error. You fix it, submit again, and then you see that there are other errors. I searched through issues, and found nothing related to this.
MR is huge, I decide to move test speed improvements and refactor to separate issue.
nikolay shapovalov → changed the visibility of the branch 3418184-improve-test-speed to hidden.
I spend some time trying to generate error without Migrate UI, but have no luck. I agree with @benjifisher to keep test as is.
testNoLinkUri()
now can be completely removed. Because <nolink><none><button>
field values added to new kernel tests for default link formatter and separate link formatter. And add new valid_entry <button>
field value for method doTestURLValidation()
.
I have few suggestions:
- improve test speed for
doTestURLValidation()
. Some changes already made at branch 3418184-improve-test-speed. - change method name to
doTestUrlValidation()
.
Should these suggestions implemented in this issue?
Set back to NR to get some feedback. IS updated.
Set to NW because want to convert to kernel testNoLinkUri()
Update MR. Added <button> <none> <nolink>
to already converted tests. And replace array filter with simple string values. Also make check for each field delta separate, and use assertEquals.
Made some research on method doTestURLValidation() and found few ways to improve tests speed.
I think some of tests can be moved to constraint validation test.
Push changes to new branch.
Not sure I follow what in testNoLinkUri is the concern about being in kernel?
Do you think it should be converted to kernel, or keep this functional?
I doubled checked testNoLinkUri()
, we are testing widget at doTestURLValidation()
for links
<nolink>
<none>
We can add <button>
to doTestURLValidation()
and then completely remove testNoLinkUri()
. Also add these cases to new Kernel tests.
If we are not going to make any changes for testNoLinkUri()
, we should convert from 3 requests to 1 request.
@nicxvan, thanks for update. I closed MR.
I moved functions
- deprecation_test_deprecated_hook
- deprecation_test_deprecated_alter_alter
to Drupal\deprecation_test\Hook\DeprecationTestHooks
class
And it become very similar to Drupal\deprecation_hook_attribute_test\Hook\DeprecationHookAttributeTestHooks
.
Should we remove module deprecation_hook_attribute_test at all?
Or is it better to remove class Drupal\deprecation_test\Hook\DeprecationTestHooks
?
nikolay shapovalov → created an issue.
LayoutBuilderUiTest::testNewExtraField doesn't test content display.
It just test layout_builder admin manage page: admin/structure/types/manage/bundle_with_section_field/display/default/layout
. And this test doesn't check entity display at all. That the reason function layout_builder_extra_field_test_node_view
can be removed.
Create issue at coder to implement check hook name at Hook attribute 📌 Add rule to check Hook attribute doesn't start with hook_ Active .
PR created https://github.com/pfrenssen/coder/pull/260
nikolay shapovalov → created an issue.
I decide to remove layout_builder_extra_field_test_node_view(), IS updated.
Looks like layout_builder_extra_field_test_node_view can be simply removed. Because we have LayoutBuilderExtraFieldTestHooks::hook_entity_extra_field_info
Great catch. Thanks. Ready for review.
MR is ready for review.
@nicxvan. Thanks for review. Meta issue is great, thanks for all your hard work.
decide to combine several hooks in one issue.
nikolay shapovalov → created an issue.
nikolay shapovalov → created an issue.
There is an issue with solving this ✨ Allow kernel tests to fail or expect logged errors Needs work .
This is gonna be solved at 📌 Replace usage of dblog module at tests with alternative Active
Next time I will use search better.
I tried to do pretty similar thing but with different approach at
📌
Replace usage of dblog module at tests with alternative
Active
.
And I am very happy about approach used at this issue. Having this at Kernel test by default will improve DX a lot, because now there are tons of different ways doing this.
I would suggest not to replace tokens at message, but keep original message and provide tokens value. Because at FormattableMarkup::placeholderFormat there are much changes, not just strtr().
And we are not checking message display but provided values.
And it much easy to search message source.
You can check example at MR 10863, but here is some code for short example.
$expected_message = [
'message' => "View display '@id': Comment field formatter '@name' was disabled because it is using the comment view display '@display' (@mode) that was just disabled.",
'context' => [
'@id' => $host_display_id,
'@name' => $field_name,
'@display' => EntityViewMode::load("comment.$mode")->label(),
'@mode' => $mode,
],
];
$this->assertTrue($logger->hasRecord($expected_message, RfcLogLevel::WARNING));
nikolay shapovalov → changed the visibility of the branch 3496184-convert-smart-default-settings to hidden.
nikolay shapovalov → changed the visibility of the branch 3496184-logger-test-class to hidden.
I created a Proxy for \ColinODell\PsrTestLogger\TestLogger to filter context of log records.
And result looks good. All \ColinODell\PsrTestLogger\TestLogger methods can be used.
Probably this is better than creating custom TestLogger from scratch.
I create helper class TestLogger. Attaching it directly in the test method looks very easy, and hasn't a lot of boilerplate.
I was thinking to use colinodell/psr-testlogger, but there are a lot of data in the log->context. In order to filter it, we can use proxy and facade approach, but I want to keep it simple. Creating custom class looks like good alternative.
Also I added rendered message to log, and TestLogger can search rendered message, this is not used in MR, but this should be useful for someone who is searching simple rendered message.
Set to NR to get some feedback.
I combined methods LinkFieldTest::testLinkFormatterQueryParametersDuplication()
and LinkFieldTest::doTestLinkFormatter()
, and move it to class Drupal\Tests\link\Kernel\LinkFormatterDisplayTest
.
method LinkFieldTest::doTestLinkSeparateFormatter()
to class LinkSeparateFormatterDisplayTest
.
method LinkFieldTest::doTestLinkTypeOnLinkWidget()
to class LinkFieldWidgetTest
.
And created base class for LinkFormatterDisplayTest, LinkSeparateFormatterDisplayTest
.
Looking for feedback.
@nicxvan thanks for review and creating CR. CR looks good.
Please take care of deprecation, if we will decide to use deprecation.
I think CR is enough, but let's wait for someone else feedback as well.
+1 for RTBC. Thanks @dillix
Converting doTestLinkTypeOnLinkWidget was not just copy and past, had to create ViewDsiplayForm and then attach field. Not sure if the tests doesn't change itself, maybe I miss something.
doTestLinkFormatter and doTestLinkSeparateFormatter looks very similar to testLinkFormatterQueryParametersDuplication before refactor. I will refactor them as well.
Thanks, LGTM.
@benjifisher thanks a lot for great review and your changes to IS. I applied suggested change to second assert and update fork to latest 11.x.
GitLab CI has a test-only job. (It is not run automatically. You have to trigger it when you want it.) We do not need a test-only branch, so I am hiding the one you added.
Thanks for sharing information about test-only job, didn't know.
@nicxvan thanks for feedback.
But as I can see colinodell/psr-testlogger is already a part of Core dependency.
I will ask for advice anyway, thanks. Just want to make some research before, to have better understanding of the topic.
Thanks for update @dillix.
I found some more issues, please fix this.
I decide that it would be great to have TestLogger to use at tests as dblog alternative.
And found that we already using \ColinODell\PsrTestLogger\TestLogger at some tests.
Another great example is \Drupal\Tests\feeds\Kernel\TestLogger and Drupal\Tests\feeds\Kernel\FeedsKernelTestBase.
Please check #17.
Made change to MR, to use Drupal service instead of container. Because of 🌱 Use \Drupal consistently in tests Needs work .
I double check LinkFieldTest.php, there methods also can be moved to Kernel:
doTestLinkFormatter - http request to create entity.
doTestLinkSeparateFormatter - http request to create nodes, but we are testing formatter. I beleive we can create nodes with code.
doTestLinkTypeOnLinkWidget - no http request, simple convert.
testNoLinkUri - http used to creates nodes. It looks likes this is test for both widget and formatter. Not sure if it's good idea to convert to kernel. What do you think?
And in order to move to Kernel we need to move renderTestEntity to trait.
I double check existing MR and changes I made is not only about moving testLinkFormatterQueryParametersDuplication() to Kernal, but also about refactoring, this is not just copy/paste. I update issue summary and title, to correspond suggested changes. And we can focus on other methods at follow-up issue.