Bug 220025 – LayoutTestFinder should be in charge of finding layout tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Sam Sneddon [:gsnedders]]

Currently, LayoutTestFinder finds the tests through: self._port.tests(paths, device_type=device_type)

That method and some related functions, instead of being under Port, should be moved to LayoutTestFinder. (Note currently the method only exists in port.base, and is never overridden, and it is unlikely it ever will be.)

[Sam Sneddon [:gsnedders]]

Created attachment 416550 [details]
Patch

[Jonathan Bedard]

Comment on attachment 416550 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416550&action=review

This patch needs a changelog.

I also think that while it's going the right direction, we probably want to take this one piece at a time.nFor example, it seems like the removal of mock_drt is needed for the change, but not strictly part of the change. I also think we've missed some fundemental building blocks of layout test discovery that are still owned by base.py after this patch, such as Base.test_key

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_finder.py:51
> +    if ext not in Port._supported_reference_extensions:

Code like this makes me think that this patch doesn't finish what it intends to do. It seems bad to separate supported_test_extensions from their reference files (and it seems bad that both of them are in base.py)

[Radar WebKit Bug Importer]

<rdar://problem/72674981>

[Sam Sneddon [:gsnedders]]

Created attachment 417085 [details]
Patch

[Sam Sneddon [:gsnedders]]

(In reply to Jonathan Bedard from comment #2)
> Comment on attachment 416550 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416550&action=review
> 
> This patch needs a changelog.

New patch! Now with the changelog I wrote before Christmas!

> I also think that while it's going the right direction, we probably want to
> take this one piece at a time.nFor example, it seems like the removal of
> mock_drt is needed for the change, but not strictly part of the change. I
> also think we've missed some fundemental building blocks of layout test
> discovery that are still owned by base.py after this patch, such as
> Base.test_key

I can certainly split out the mock_drt change if you want, at least.

As for Base.test_key, I'm pretty certain that remained where it was due to it also being referenced in Scripts/webkitpy/layout_tests/controllers/manager.py; in principle it would be nice if we made the API of the LayoutTestFinder such that it returned them in "natural" order hence no further resorting was needed, but I was trying to avoid overcommitting (and trying to avoid having to understand the manager right now!), though it does look like the manager doesn't do too much with the list, so maybe I can just change that now?

> 
> > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_finder.py:51
> > +    if ext not in Port._supported_reference_extensions:
> 
> Code like this makes me think that this patch doesn't finish what it intends
> to do. It seems bad to separate supported_test_extensions from their
> reference files (and it seems bad that both of them are in base.py)

To summarise what we discussed in person, my longer term plan was to migrate all the expectation-finding code into the LayoutTestFinder as well. Currently we build up TestInputs in the manager, add the reference_files to them in layout_test_runner, and then construct a DriverOutput with all the relevant expectations in single_test_runner. This doesn't really make sense to have split across three places.

At the moment, after this patch, finding expectations (references among them) remain with the Port, but that should be changed in a later patch.

[Jonathan Bedard]

Comment on attachment 417085 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417085&action=review

r=me (even though Darin already r+ed it, want to be clear that all my concerns have definitely been addressed)

> Tools/ChangeLog:35
> +        (MockPort.__init__): Deleted.

Nit: We can make this shorter by just saying "(MockPort): Deleted." instead of listing each function we deleted.

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_finder.py:75
>          self._port = port

Based on the conversation Sam and I had today, I think we want a FIXME here to indicate that we think the port object should not be passed to LayoutTestFinder. We aren't there yet, and a patch to get us all the way would be too large, but seems worth mentioning.

[Sam Sneddon [:gsnedders]]

Created attachment 417190 [details]
Patch

[EWS]

Committed r271244: <https://trac.webkit.org/changeset/271244>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417190 [details].