Page MenuHomePhabricator

Flow Browser tests fail with "Error modules: ext.thanks.flowthank." on beta
Closed, ResolvedPublic

Description

Several Flow browser tests are failing with "Error modules: ext.thanks.flowthank."

The failing step is "page has no ResourceLoader errors" from mediawiki_selenium. Essentially, mw.loader.getState( 'ext.thanks.flowthank' ) returns 'error' in those cases.

Executing that command manually on beta always return 'ready'.

I cannot reproduce the error by running the tests locally against a local or mw-vagrant server but I get the error about 50% of the time when running the tests locally against beta.

Event Timeline

SBisson claimed this task.
SBisson raised the priority of this task from to High.
SBisson updated the task description. (Show Details)
SBisson added a subscriber: SBisson.

The error in the console is "$thankLink.findWithParent is not a function".

ext.thank.flowthank.js:13 (Thank extension) uses $.findWithParent defined in jquery.findWithParent.js (Flow extension) without an explicit dependency. When it's not loaded in the right order, it breaks.

I see a few ways to solve this:

  1. Create a jquery.findWithParent module in Core and have both Flow and Thank depend on it. It seem relatively quick and simple, assuming the guardians of Core see this module as useful and not pollution.
  2. Make Thank an optional depdendency of Flow instead of the other way around like it is now. Change Thank so it exposes a generic interface that Flow can use to attach it to it's things (Replies) that can be thanked. It's more work but it seem to make sense. Flow has a similar relationship with Echo.
  3. Add an explicit dependency from Thank to ext.flow.jquery.findWithParent. Probably a bad idea as Thank would refuse to work without Flow.

I would go with #1. Am I missing something? What do you think @mattflaschen, @Legoktm, @Catrope?

Change 248069 had a related patch set uploaded (by Sbisson):
New module: jquery.findWithParent

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

Change 248070 had a related patch set uploaded (by Sbisson):
Depend on jquery.findWithParent from Core

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

Change 248072 had a related patch set uploaded (by Sbisson):
Depend on jquery.findWithParent from Core

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

  1. Create a jquery.findWithParent module in Core and have both Flow and Thank depend on it. It seem relatively quick and simple, assuming the guardians of Core see this module as useful and not pollution.
  2. Make Thank an optional depdendency of Flow instead of the other way around like it is now. Change Thank so it exposes a generic interface that Flow can use to attach it to it's things (Replies) that can be thanked. It's more work but it seem to make sense. Flow has a similar relationship with Echo.
  3. Add an explicit dependency from Thank to ext.flow.jquery.findWithParent. Probably a bad idea as Thank would refuse to work without Flow.

This would only be a dependency of the ext.thanks.flowthank module, which already requires Flow, so it wouldn't introduce a hard dependency AFAIS.

3 is probably easiest, I'd rather not do 2, and 1 could be a good idea if it meets core guidelines.

3 is probably easiest, I'd rather not do 2, and 1 could be a good idea if it meets core guidelines.

I've implemented 1 in the three patches above. I'll do 3 if 1 is not acceptable.

I see a few ways to solve this:

  1. Create a jquery.findWithParent module in Core and have both Flow and Thank depend on it. It seem relatively quick and simple, assuming the guardians of Core see this module as useful and not pollution.
  2. Make Thank an optional depdendency of Flow instead of the other way around like it is now. Change Thank so it exposes a generic interface that Flow can use to attach it to it's things (Replies) that can be thanked. It's more work but it seem to make sense. Flow has a similar relationship with Echo.
  3. Add an explicit dependency from Thank to ext.flow.jquery.findWithParent. Probably a bad idea as Thank would refuse to work without Flow.

I'd recommend detaching this method in Flow from the jQuery prototype. Have it live as utility method in a Flow interface (not a jQuery interface). Whether it is publicly exposed depends on Flow's own use case.

Thank can ship its own variant of the utility method. It's simple enough. Sharing this costs more than it gains in terms of complexity and maintainability. Especially considering that the need of such method likely originates from technical debt that will eventually be resolved.

Change 249095 had a related patch set uploaded (by Sbisson):
Add jquery.findWithParent to Thanks

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

Thank can ship its own variant of the utility method. It's simple enough. Sharing this costs more than it gains in terms of complexity and maintainability. Especially considering that the need of such method likely originates from technical debt that will eventually be resolved.

I completely agree. I'm usually the one arguing that sometimes duplication is better than coupling. I lost sight of the big picture here.

I'd recommend detaching this method in Flow from the jQuery prototype. Have it live as utility method in a Flow interface (not a jQuery interface). Whether it is publicly exposed depends on Flow's own use case.

I think it's a legitimate jquery extension, or at least not worst than most. Now, whether jquery extensions are legit in the first place is debatable ;) But exposing it as Flow's interface doesn't seem like a good idea. It's a conversation extension, not a DOM manipulation library.

Change 248069 abandoned by Sbisson:
New module: jquery.findWithParent

Reason:
Replaced by Id74e77b5fb81d5da8cb6dd97fd1b90e5d974ae82

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

Change 248070 abandoned by Sbisson:
Depend on jquery.findWithParent from Core

Reason:
Replaced by Id74e77b5fb81d5da8cb6dd97fd1b90e5d974ae82

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

Change 248072 abandoned by Sbisson:
Depend on jquery.findWithParent from Core

Reason:
Replaced by Id74e77b5fb81d5da8cb6dd97fd1b90e5d974ae82

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

Change 249095 merged by jenkins-bot:
Add jquery.findWithParent to Thanks

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