Problem/Motivation

I originally started working on this as an exploration of converting functional tests to kernel after the addition of drupalGet(). I only found one that seemed to be a good candidate, LinkFieldTest::testLinkWidgetCaughtExceptionEditingInvalidUrl(). The others integrate with other functional base subclasses or submit forms.

Then I found LinkFieldUITest and I was kind of disturbed by it. It does so much. It's no wonder it has to be marked as slow. I'm not going to criticize whoever has written on it over the years. I checked the history. It's picked up more and more stuff over time. The problem that I see with it is that it effectively tests the field and field_ui systems. It doesn't have focus on verifying the Link plugin logic that is sent to those systems. The test does this by loading all different pages within the field systems to check for Link's stuff. This is why it's so slow! And this is pointless unless there's an integration problem. We need to refocus the tests on checking the API.

Proposed resolution

Move LinkFieldTest::testLinkWidgetCaughtExceptionEditingInvalidUrl() (functional) to LinkFieldWidgetTest::testLinkWidgetCaughtExceptionEditingInvalidUrl() (kernel).

Break up LinkFieldUITest into focused Kernel and Unit tests. In my local environment, the time it takes to run LinkFieldWidgetTest was 39 sec. The time it takes to run the new kernel and unit tests is 4 sec. So it's a nearly 10 times improvement.

Improve test coverage where possible.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3612850

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

dcam created an issue. See original summary.

dcam opened merge request !16416

dcam’s picture

dcam commented
Status: Active » Needs review

I did a self review to explain where everything has moved. Check the MR comments.

acbramley’s picture

acbramley commented

This is awesome!

Testing locally, the widget test went from 45 seconds to just under 4 seconds with a hugely simplified setup by being able to reuse ::entitySetUp

As for LinkFieldUITest - locally that took just over 1 minute to run the full class. The LinkFieldWidget kernel test takes 13 seconds for the full class including existing and new test cases.

Overall this looks like a huge improvement, the only thing worth mentioning is we are no longer testing an end-to-end flow of the widget in a real form/page. I think losing the Field UI bits is probably fine, but we may want to keep at least 1 functional test case that proves the widget works in a real form/page?

dcam’s picture

dcam commented

...we may want to keep at least 1 functional test case that proves the widget works in a real form/page?

I think that LinkFieldTest::doTestLinkTitle() covers that. Most of it is focused on validating the title visibility settings, but there is a test for a successful entity form submission with the URI and title subfields too. It renders the test entities instead of visiting their pages in the browser. Is there a problem with that method of validating output?

Do you think we need another test beyond what we already have there?

acbramley’s picture

acbramley commented
Status: Needs review » Reviewed & tested by the community

Ah yeah I missed that, I think that's enough.

mstrelan’s picture

mstrelan commented

@dcam, regarding tests with submitForm, I'd love your feedback on the MR on #3566881: Add a submitForm() method to HttpKernelUiHelperTrait. It's working well for me in #3609454: Convert functional tests in system module with submitForm to kernel tests, and I'm also working on converting many tests in workspaces module, where it seems to be working well too. Would be great if you can try it with link module.

dcam’s picture

dcam commented

...I'd love your feedback on the MR...

I put it in my queue to review.

  • catch committed 6a420f1d on main 
    task: #3612850 Overhaul Link functional tests

By: dcam
By: acbramley
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

This looks great.

The first (and second) wave of simpletests that got added was a scramble to add coverage from a standing start, so these kind of 'multi-purpose' tests that cover unrelated things was somewhat on purpose at the time, apart from also the limitations of simpletest, but it stored up a lot of problems for later (having to update dozens of tests when changing something that should rightly be covered by one or two, CI runtimes) that we're only really making a dent in now.

This doesn't cherry-pick to 11.x - moving to fixed but we could backport if we want to. I think the time is probably better spent converting more functional tests to kernel tests in main though.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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