Page MenuHomePhabricator

OAuth extension REST tests must not instantiate a Router
Closed, ResolvedPublic

Description

The core Router class is not @newable thus extensions must not instantiate a router via a constructor. OAuth extension tests violate this constraint which limits our ability to change the Router constructor signature.

Core provides HandlerTestTrait that could be used to write unit tests for OAuth, and integration tests could be written using api-testing framework. The existing REST endpoints tests in the OAuth extension should be rewritten using these two abstractions and about instantiating a non-newable Router.

Event Timeline

Core provides HandlerTestTrait that could be used to write unit tests for OAuth, and integration tests could be written using api-testing framework. The existing REST endpoints tests in the OAuth extension should be rewritten using these two abstractions and about instantiating a non-newable Router.

Is this trait meant to be used by extensions? T263904: Are traits part of the stable interface?
Separate from the default status of traits as stable or not, I would expect files and traits that are part of the testing system to be less intended to be used by extensions. I suggest leaving a note on the trait if it is going to have more external uses, pending the decision at T263904

We should probably look at that trait again and see if we can make it more 'stably' and frameworky. Perhaps we could convert it to a new TestCase base class too.

After looking over this with @Hknust , @Clarakosi , and @ArielGlenn , we have a few thoughts:

The simplest way to fix the current issue is to create something like a MediaWikiRestTestCase or MediaWikiRouterTestCase that inherits from MediaWikiTestCase. This new base class would implement a function that basically contained lines 72-83 (or maybe 84) of the current EndpointTest::testViaRouter from the OAuth extension. This moves creation of the Router object into core and solves our immediate problem in a very safe and straightforward way. We could do this, close this ticket, and move on. There were no objections to this plan in our discussion.

However, the longer term downside of this plan is that it isn't very reusable. Our REST tests inherit from different base classes depending on the test. For example, LanguageLinksHandlerTest) inherits from MediaWikiIntegrationTestCase and SearchHandlerTest inherits from MediaWikiUnitTestCase. This is exactly what the HandlerTestTrait solves, because it can be used by various tests regardless of where they live in the inheritance tree. So if we end up putting very much useful functionality into the new base class, we'll like run into disappointment later when certain tests can't get to it.

So we remain interested in the T263904: Are traits part of the stable interface? discussion. If we determine that traits are part of the stable interface, we might later convert the new base class to a trait.

We also noticed that the "handler" tests in the OAuth extension are really more like integration tests, and would probably be better suited for the Integration Test Framework. But we would still want unit tests to exercise the individual handlers.

So as a followup task, we recommend adding new true integration-level tests, and also writing unit tests for the individual handlers in the REST API. That's a large enough effort it seems worth breaking out. Especially as this current ticket is blocking a merge on https://gerrit.wikimedia.org/r/c/mediawiki/core/+/632800.

Change 655948 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] Split title mocking methods out of HandlerTestTrait.

https://gerrit.wikimedia.org/r/655948

Pchelolo added a subscriber: holger.knust.

After the above patch is merged, the HandlerTestTrait becomes stable and should be used in OAuth extension tests instead of instantiating Router.

Change 655948 merged by jenkins-bot:
[mediawiki/core@master] Split title mocking methods out of HandlerTestTrait.

https://gerrit.wikimedia.org/r/655948

Pchelolo raised the priority of this task from Medium to High.Jan 19 2021, 6:16 PM

Change 657379 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/core@master] Add executeHandlerAndGetReponse to HandlerTestTrait

https://gerrit.wikimedia.org/r/657379

Change 657386 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/extensions/OAuth@master] Avoid creating Router inside tests and update all tests accordingly

https://gerrit.wikimedia.org/r/657386

Change 657379 merged by jenkins-bot:
[mediawiki/core@master] Inject $user for validation instead of creating it inside Trait

https://gerrit.wikimedia.org/r/657379

Change 657386 merged by jenkins-bot:
[mediawiki/extensions/OAuth@master] Avoid creating Router inside tests and update all tests accordingly

https://gerrit.wikimedia.org/r/657386