Attempt to move large parts of LinkFieldTest to a kernel test

nikolay shapovalov → made their first commit to this issue’s fork.

I move method testLinkFormatterQueryParametersDuplication to new Kernel test case.
I create draft MR #10286.

It looks like method can be simplified, and I tried to work in that way.
But I didn't finish it yet.
You can find changes at 3418184-refactor-wip branch.

nikolay shapovalov → closed merge request !10286

I was able to finish refactoring.
MR #10286 closed, use MR #10293.

MR #10293 is waiting for review.

Using data provider has downside.
Each test case from data provider executes setUp() method.
And it means that instead creating single entity, it creates entities 6 times.
Please let me know if you have an idea to improve.

I decide to remove dataProvider.
Using dataProvider takes 10 seconds, and without it takes 2 seconds.

Thanks @andypost for the review. MR updated and ready for review.

Left a comment on the MR.

Thanks for review. MR updated. Ready for review.

Feedback appears to be addressed.

I made several suggestions for comments in the MR. I also think this needs a title update because this is no longer 'attempting' to move code to a Kernel test. Maybe 'Convert testLinkFormatterQueryParametersDuplication to a Kernel test'?

Are there other test methods that could be moved to a kernel test?

Thanks for review and feedback, I updated issue title.

Are there other test methods that could be moved to a kernel test?

Do you mean other methods at LinkFieldTest.php?
I didn't check other tests yet, because I believe this issue is kind of follow-up for 🐛 Query string duplications Fixed .

@quietone thanks for your feedback. MR is ready for review.

If you see other tests in this file that can be converted think it would be in scope of this ticket.

Thanks for keeping this going! It’s close

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.

Made change to MR, to use Drupal service instead of container. Because of 🌱 Use \Drupal consistently in tests Needs work .

Please check #17.

Would say whatever easily can be converted lets do here. And document in the summary why others were not.

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.

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.

Not sure I follow what in testNoLinkUri is the concern about being in kernel?

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.

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.

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.

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.

nikolay shapovalov → changed the visibility of the branch 3418184-improve-test-speed to hidden.

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.

Update IS.

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.

Going to leave in review but what's the purpose of breaking out the different formatter settings into their own files?

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?