Page MenuHomePhabricator

Document/estimate existing JavaScript code coverage in MobileFrontend
Closed, ResolvedPublic5 Story Points

Description

Before embarking on the refactor of MobileFrontend, we'd like to get a sense of how much code coverage we currently have inside the MobileFrontend codebase to show the value of this work. At the end of the project and at key points we'll observe how this has changed.

Acceptance criteria

Note: There are very few unit tests in Minerva so we purposely exclude this from those measurements.

Event Timeline

Niedzielski updated the task description. (Show Details)Jun 20 2018, 2:05 PM
Jdlrobson set the point value for this task to 5.Jun 26 2018, 4:51 PM
Jdlrobson moved this task from Inbox to Next up on the User-Jdlrobson board.Jun 27 2018, 8:34 PM
Vvjjkkii renamed this task from Document/estimate existing JavaScript code coverage in MobileFrontend to epaaaaaaaa.Jul 1 2018, 1:02 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed the point value for this task.
Vvjjkkii removed a subscriber: Aklapper.
CommunityTechBot renamed this task from epaaaaaaaa to Document/estimate existing JavaScript code coverage in MobileFrontend.Jul 2 2018, 1:41 PM
CommunityTechBot raised the priority of this task from High to Needs Triage.
CommunityTechBot set the point value for this task to 5.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added a subscriber: Aklapper.
Jdlrobson moved this task from 2017-18 Q4 to Upcoming on the Readers-Web-Backlog board.
Jdlrobson triaged this task as High priority.Jul 5 2018, 6:58 PM
pmiazga claimed this task.Jul 11 2018, 12:45 PM
pmiazga moved this task from To Do to Needs Code Review on the Readers-Web-Kanbanana-Board-Old board.
Jdlrobson added a comment.EditedJul 25 2018, 2:48 AM

Just back from vacation. I'm hearing rumours this might not be possible but I'd be interested to hear a write up! (note spike portion of this task was strictly timeboxed at 4hrs precisely for this reason so it's okay to fail!)

Findings

I spend some time and tried to run the coverage tests for MobileFrontend with no results. The existing architecture (karmaProxy) do not allow us to run coverage tests, and the code itself is highly coupled with MediaWiki core javascript platform and it's not possible to run any of unit tests in total isolation.

The main blocker is that all JavaScripts are loaded via ResourceLoader (requests to load.php). If we want to make this work we need to:

  • convince ResourceLoader to load all modules separately (via debug/flag), some modules consist of many JS files which makes it even more complex (example: mobile.watchlist module uses WathListGateway.js and Watchlist.js)
  • convince nyc/istanbul to log coverage for the load.php requests
  • create nyc/istanbul bindings for the local files, and match the coverage for load.php requests with existing files (please see point 1 - some modules consist of multiple JS files) Those changes have to be made in the MediaWiki core ( in Gruntfile.js,m plus probably couple more files ) so probably we won't be allowed to commit those to the repository.

I also tried the second approach, run tests in the console in full isolation (not via MediaWiki QUnit suite) and I stumbled across way too many issues. First I got into version conflicts between node jQuery package,the jQuery package we use, and also some problems with OOUI initialization., Most of that stuff can be solved by using wikimedia-setup.js file (@Jdlrobson thanks for the link), but then we hit lots of undefined errors when code is trying to use uninitialized mw.* properties. This probably can be also avoided by doing a proper call to load.php and load all required MediaWiki modules which should properly populate the global.mediawiki variable.
Each unit test probably will require a fresh instance of mobile.init so we do not keep state between different tests (currently tests do not run in full isolation which makes me sad).

OOUI creates the code coverage

Yes, OOUI JS library and it doesn't rely on the MediaWiki codebase. The OOUI qunit tests do not use karmaProxy, and the modules are loaded by Javascript (see Gruntfile.js). Those files are loaded manually, not via load.php, thus the coverage is possible. Also if you check the OOUI code, it doesn't use any properties of the mediawiki object.

What to do now

IMHO, trying to rewrite MobileFrontend tests to become testable without the MediaWIki instrumentation is a suicide mission, it will require lots of time (after a week we can check how far we got) and the task itself is not that exciting. I would propose to create a basic infrastructure to run QUnit tests same way as we those in the Popups repository and refactor a couple tests as a proof-of-concept. Then, when we start working on the MobileFrontend code, before touching any module, first we will have to refactor given module unit tests to be able to run those in the console. Also, this approach should prepare us better for the specific module refactor - first we will need to understand the test, module dependencies and the what is the true role of given module in the MobileFrontend codebase.

Questions

ATM I have no idea how to create the test cases in the way that it is possible to run those both via npm run test:node and visiting the Special:JavascriptTest page. Currently, the Popups Qunit test suite is not available in Special:JavasScriptTest nor in via grunt test command executed in MediaWiki repository.

@Jdlrobson if we want to do the Javascript coverage anyway, we need to re-estimate this task as it's not a 5 pointer, the risk is small but there is lots of work, IMHO too much for one task. I'm voting to close this task, and create new tasks - or to create an initial environment to run QUniut tests same way as we in Popups repository and then create a set of tasks to port existing unit tests into new test runner.

Jdlrobson added a subscriber: pmiazga.

@nray @Jdrewniak @pmiazga @Niedzielski given these challenges, it feels like we should do one of the following

  1. speed up unit tests THEN increase code coverage

Refrain (even though it is extremely tempting) from writing new unit tests until code is using webpack and all tests are run in node-qunit.
We'd probably want to reserve time to work on this project, freezing feature development and the the writing of unit tests (and only dealing with unbreak now bug reports) until the porting is done.
When this is done we'd finish off T195478

  1. speed up unit tests WHILE increasing code coverage

After porting individual unit tests to node-qunit and before beginning to write the new unit tests, we'll take a snapshot of test coverage. We'll compare this to the coverage at the end.

Any preferences on which?

Jdlrobson updated the task description. (Show Details)Jul 30 2018, 9:32 AM

Refrain (even though it is extremely tempting) from writing new unit tests

We can always add QUnit.skip to tests we don't want to measure later too.

@Jdlrobson, I'm down for #2 where it makes sense. This might look like two patches per port (one to port, one to improve / add).

In my view, we have enough constraints just trying to refactor patches for immediate merging into master without introducing regressions. I'm not big on the idea of inhibiting progress further so that we can measure how successful the code coverage is. We should treat it as an estimation and that we /can't/ practically make even an approximate measurement says something about the state of the codebase as it is now. I propose we aim to develop as efficiently as possible and report metrics where pragmatic but it should not block opportunistic development wins such as adding tests that may have the immediate user impact of preventing bugs from hitting prod.

I think that #2 is the better approach, we could do small incremental changes, get a better knowledge whats going on, Rewriting tests just to run them via console qunit without an easy way of measurement whats done/whats improved sounds like work that doesn't give any gratification (eg - I rewrote the test, it runs, but I don't know if everything is properly tested). Also if we try to speed up unit tests and increase code coverage at the same time we might find out that we did some mistakes when refactoring.

nray added a comment.Jul 31 2018, 8:34 PM

I'm also in favor of #2 and doing an incremental change. #1 has the disadvantage of blocking new development for an unknown duration of time and seems like it introduces more context switching than #2 making it more difficult to do in practice. I feel like my memory of how a test works / is trying to accomplish will be clearest right after I port it thus facilitating the addition of new tests (#2), whereas with #1, I think I would forget a number of details with the tests by the time of the code coverage increase, and I would need to go back to review the tests.

So how about we do 2 but tag tests in some way so as stephen says we can identify new tests and skip them to get a sense of code coverage? Tagging could be as simple as prefixing test name with a keyword such as rewrite: or tagging old tests with old.

Jdlrobson moved this task from Next up to Done on the User-Jdlrobson board.Aug 10 2018, 8:55 PM
Jdlrobson closed this task as Resolved.Aug 14 2018, 5:10 PM

I'm resolving this, but we need to follow up and talk about https://phabricator.wikimedia.org/T197637#4492960 during next meetup so we can check we are on the same page going forward.