Page MenuHomePhabricator

Separate implementation from configuration in Wikibase's Vue version comparison test
Closed, ResolvedPublic

Description

Following up on solution introduced for T259468, the test ensuring that Wikibase Vue microfrontends use the same version of vue libraries as are being ultimately served to client by Mediawiki could be further improved by separating the checking script from its configuration: details on what files should be compared.

Possible approach has been outlined in the comments on https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/645029/3/client/data-bridge/tests/mwlibs.js#15

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 655641 had a related patch set uploaded (by Noa wmde; owner: Noa wmde):
[mediawiki/extensions/Wikibase@master] Wikibase: make Vue version compare test more dynamic

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

Change 657058 had a related patch set uploaded (by Noa wmde; owner: Noa wmde):
[mediawiki/extensions/Wikibase@master] data-bridge: replace mwlibs-testing with lib-version-check

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

Change 657071 had a related patch set uploaded (by Noa wmde; owner: Noa wmde):
[mediawiki/extensions/WikibaseLexeme@master] WikibaseLexeme: replace mwlibs-testing with lib-version-check

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

Change 657074 had a related patch set uploaded (by Noa wmde; owner: Noa wmde):
[mediawiki/extensions/Wikibase@master] Tainted-Ref: replace mwlibs.js with lib-version-check

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

Change 657058 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] data-bridge: replace mwlibs-testing with lib-version-check

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

Change 657071 merged by jenkins-bot:
[mediawiki/extensions/WikibaseLexeme@master] WikibaseLexeme: replace mwlibs-testing with lib-version-check

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

Change 657565 had a related patch set uploaded (by Noa wmde; owner: Noa wmde):
[wikibase/termbox@master] Termbox: replace mwlibs.js with lib-version-check

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

Change 657565 merged by jenkins-bot:
[wikibase/termbox@master] Termbox: replace mwlibs.js with lib-version-check

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

Change 657074 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Tainted-Ref: replace mwlibs.js with lib-version-check

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

Change 655641 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Wikibase: remove mwlibs-testing and add lib-version-check

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

The introduced changes are a good step in the right direction - they separate implementation from configuration for the tests and the dedicated library is a nice touch as it keeps this concern out of the individual products and allows for it to be centrally maintained.

There is a very much related aspect in the original comment which apparently wasn't taken into account in the changes (granted, already in this ticket's headline), however.

The packages for which version checks are run in the tests are those and exactly those which are "externalized" from the builds. Which packages this are is currently configured in the respective vue.config.jss (e.g.). If one changes, so should the other - a task which humans are notoriously bad at even in case they know they are expected to.

So instead of centering the package.json entry (e.g.) around the idea of "configuring a test", centering it around the "domain of externalization" would remove a possible source of error and would nicely illustrate how this (arguably non-standard solution) relates to the product it lives in. The test would be one consumer and the externalization configuration (in vue.config.js) would be another. The change in role of the configuration in package.json should then probably be reflected in the naming, e.g.

{
  "name": "data-bridge",
  ...
  "config": {
    "externals": {
      "vue": {
        "version-check-url": "https://raw.githubusercontent.com/wikimedia/mediawiki/#{ZUUL_BRANCH}/resources/lib/vue/vue.common.prod.js"
      },
      "vuex": {
        "version-check-url": "https://raw.githubusercontent.com/wikimedia/mediawiki/#{ZUUL_BRANCH}/resources/lib/vuex/vuex.js"
      }
    }
  },

(I could also see lib-version-check occupy a dedicated node in this structure for extensibility instead of only the url, but that sure is an implementation detail)

No ACs here so I'm not exactly 100% sure what I need to verify.
Pinging @WMDE-leszek as the author and @noarave and @Lucas_Werkmeister_WMDE as the 2 people that touched this the most.
This should also take into account what was said by @wiese above.

Change 661077 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] bridge: read externals config from package.json

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

Change 661077 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] bridge: read externals config from package.json

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

@Lucas_Werkmeister_WMDE's fixes this for data-bridge, I guess it would make sense to do this for the other projects too for consistency purposes. Moving back to To Do.

Well, I think there are also other issues? Specifically, the Tainted References package.json configures a remoteVersion for both vue and vuex, but its vue.config.js only marks vue as external – vuex would be done in T250267. (This is, I suppose, exactly the kind of inconsistency that centralizing the config is meant to avoid.)

But I’d say that kind of cleanup is beyond the skope of this task. I’ll create another one.

Done: T273881: Unify external library configuration for Wikibase projects

I’ll move this to Verification and suggest that it’s closed, since the implementation and configuration are now separated. I’ll propose the other task for the next tech prio session.