Page MenuHomePhabricator

[Bug] Popup build synchronization tests are not failing when they ought to
Closed, ResolvedPublic3 Story Points

Description

The Popups build check is permitting the source inputs and build outputs to become out of sync. This is very dangerous as build products are not code reviewed and arbitrary code could be shipped to prod unknowingly. A failed attempt to fix the issue is here.

Observed behavior

The patch Use getPageviewToken api does not update the built assets.

npm run check-built-assets when run does not throw an error code meaning Jenkins does not catch this mistake and allows the possibility of not deploying the changes that have been made.

Acceptance criteria

  • Any change which updates the code in resources/dist/ should fail Jenkins checks if that code has not been committed.

Event Timeline

Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptAug 24 2018, 3:24 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Jdlrobson raised the priority of this task from Normal to High.Aug 28 2018, 12:06 AM
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Needs Prioritization to Upcoming on the Readers-Web-Backlog board.
phuedx added a subscriber: phuedx.Aug 28 2018, 2:32 PM

I would go so far as to say that this is UBN!

Change 456028 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Update package.json to ensure assets get built on every commit

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

In addition to @Jdlrobson's patch, @Niedzielski had a similar WIP here
https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Popups/+/454566/

In the comments, Stephen mentioned off-hand that this might be an issue with different NPM versions.
I tested this today, and it appears that it is!

Using nvm, I switched to Node V6 (lts/boron -> v6.14.4), deleted my node_modules folder, did an npm install, and ran npm -s run build
The output was different than when I ran the same process with Node V9.

These might be differences in the minifier. Regardless, I don't think we can change this behaviour unless we run an older version of Node locally, or CI runs a newer version.

Also of note, the current script runs git diff -q resources/dist I couldn't find any references to -q in the git docs, only --quiet.

Change 456028 abandoned by Jdlrobson:
Update package.json to ensure assets get built on every commit

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

Change 456028 restored by Jdlrobson:
Update package.json to ensure assets get built on every commit

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

Change 456028 abandoned by Jdlrobson:
Update package.json to ensure assets get built on every commit

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

Change 456028 restored by Jdlrobson:
Update package.json to ensure assets get built on every commit

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

Change 456028 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Update package.json to ensure assets get built on every commit

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

Jdlrobson removed Jdlrobson as the assignee of this task.Sep 5 2018, 8:03 PM

Can skip QA, design review as a technical task.
Need a signer-offer to submit a naughty patch and check that Jenkins refuses it. Any volunteers?

Change 459594 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/Popups@master] Do not merge: test Node v10.9.0 built assets

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

Change 459597 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/Popups@master] Do not merge: test unbuilt inputs

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

Change 459594 abandoned by Niedzielski:
Do not merge: test Node v10.9.0 built assets

Reason:
Success! A build product difference causes the patch tests to fail:

18:18:26 Binary files a/resources/dist/index.js and b/resources/dist/index.js differ
18:18:26 diff --git a/resources/dist/index.js.json b/resources/dist/index.js.json

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

Change 459597 abandoned by Niedzielski:
Do not merge: test unbuilt inputs

Reason:
Abandon! The build outputs differ:

18:30:25 diff --git a/resources/dist/index.js b/resources/dist/index.js
18:30:25 index 80895dd..32c5eac 100644
18:30:25 Binary files a/resources/dist/index.js and b/resources/dist/index.js differ
18:30:25 diff --git a/resources/dist/index.js.json b/resources/dist/index.js.json
18:30:25 index ab577b5..151463c 100644
18:30:25 Binary files a/resources/dist/index.js.json and b/resources/dist/index.js.json differ

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

Niedzielski closed this task as Resolved.Sep 10 2018, 6:32 PM

I'm not sure what the problem is. Are you saying I can't make a commit to Popups unless I have Node v6.11.0? If that is the case, it should be documented in ALL CAPS in the readme. 😉

you can make a commit, but you if you run npm run build the version is important.
It seems the error message is not being broadcast well enough.
I don't think adding to the README makes sense, as we already make the node version clear inside package.json and .nvmrc as well as the script that fails.
We should probably update the README to ensure that the correct version of Node is being used and that we recommend using nvm.

For what it's worth, @zeljkofilipin, I think this problem will go away in time. Once NPM v5 is deployed in CI, lockfiles will be honored and hopefully we won't have build differences across newer Node.js / NPM versions.

you can make a commit, but you if you run npm run build the version is important.

No, I could not make the commit. See T203591#4562169. Popups has a precommit git hook and it fails on my machine, aborting the commit. All I wanted to do is add a dev dependency to package.json (one line change), but I was not even able to make a commit.

It seems the error message is not being broadcast well enough.
I don't think adding to the README makes sense, as we already make the node version clear inside package.json and .nvmrc as well as the script that fails.
We should probably update the README to ensure that the correct version of Node is being used and that we recommend using nvm.

I might just be more ignorant than an average contributor, but I've sent minor patches to many repositories and I can't remember ever being unable to make a commit.

I was not aware that node version can be specified in package.json and I have never used nvm, so I've ignored .nvmrc. Now that I know that, it's obvious. I did check readme file but I could not find any special requirements for the repo.

For what it's worth, @zeljkofilipin, I think this problem will go away in time. Once NPM v5 is deployed in CI, lockfiles will be honored and hopefully we won't have build differences across newer Node.js / NPM versions.

All problems will go away in time. 😄

The problem with the commit is the pre-commit hook (which runs the npm run build command) is failing. You should be able to disable it to not run into this problem.

Is there a task tracking NPM v5 being used in CI? I want to work out what the trade-offs here are for making this better or waiting for that. It's clearly not ideal right now and is annoying for us too.