Issue 6890 - run npm tests in parallel (PENDING)

.gitlab-ci.yml

Index: .gitlab-ci.yml
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 4109a810e68940c367cffb763069bc325ebf23d2..5e721531da9c250a406c2eadfabc7e024dfcf245 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -14,12 +14,37 @@
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 stages:
-  - test_ext
+  - prepare
+  - test
 
-qunit:gecko:
-  stage: test_ext
+prepare-dependencies:
+  stage: prepare
   script:
     - mkdir -p .git/info
     - pip install --user Jinja2 cryptography
+    - python ensure_dependencies.py

[Sebastian Noack, 2018/08/28 11:22:55]

I wonder whether this should rather go in pretest in package.json? The advantage
would be that we'd also make sure the dependencies are there and up to date when
running "npm test" manually during development.

[tlucas, 2018/08/28 12:21:59]

Since "python build.py" - which is called anyway in the current pretest script /
in the mocha tests, which are yet to be landed - calls ensure_dependencies.py
anyway we are sure, that everything is up to date.

Furthermore, this implies that in the actual jobs below we are also sure that
dependencies are installed and up to date. There is, unfortunately, no
guarantee, that caching will succeed in time for subsequent jobs. Which brings
me to: i should call "npm install" again in the jobs below, changed that.

[Sebastian Noack, 2018/08/28 12:50:57]

After your changes running the tests as well on Chrome, build.py isn't invoked
by pretest anymore, but from within the test suite. The test suite, however,
imports code from adblockpluscore.

[tlucas, 2018/08/29 08:14:27]

Gotya. I moved "python ensure_dependencies.py" to "pretest" accordingly - as
well as to the tests' local before_scripts. (IMHO moving
"ensure_dependencies.py" into the "pretest" script is rather a workaround for
local dependency management - and in CI i don't want to have that management
scattered all over the place / hidden in some package.json files. and FWIW,
since both "npm install" and "ensure_dependencies" are essentially noops, IMO
this doesn't hurt.)

[Sebastian Noack, 2018/08/29 21:03:11]

Now, we are running ensure_dependencies.py 3 times?! Once in the prepare stage,
once in the before_script, and then again in "pretest" in packages.json.
(Actually, it's 4 times, if you count build.py calling it again, but we cannot
really avoid that). But running it in the prepare stage and before_script seems
redundant to me. If it's just to make it more visible, I don't think this is
justified, or is there any other benefit?

As for "npm install", it's even worse, since it's quite expensive even if there
is nothing to do. Every time you "npm install" it talks to a bunch of servers to
check for updates.

[tlucas, 2018/08/30 11:05:58]

Let me try to clarify the current state (chronologically):

1) The first invocation of "ensure_dependencies.py" (i.e. while preparing the
cache) is merely an optimization step, though not necessary this speeds up all
subsequent jobs immensely. Same applies to "npm install"

2) The second call to "ensure_dependencies.py" (and "npm install") from the
jobs' local "before_script" is to actually ensure that all dependencies are
installed. Mind that there is *no guarantee* (Winsley once encountered this
IIRC), that the cache is actually available when a subsequent job starts
running.

3) The third call (from within "pretest") is now a noop besides "npm install" in
a CI environment (since we are now sure that everything is installed), but
necessary for local development

4) The fourth call is triggered by invoking "build.py", this we can not change /
don't want to change.

So the steps worth looking at are 1) and 2), where 1) only is necessary while
talking about parallel jobs with shared dependencies (which is true for us) and
step 3).

For step 3), we could run "build.py devenv -t <platform>" in the "all.js"
script, *before* actually requiring the code from adblockpluscore (meaning we
could get rid of the "pretest" section, when installing and requiring in order)

For steps 1) and 2) -> 
The only real option would be not installing the dependencies in
"prepare-dependencies" (but for every job individually) since there is no
guarantee that caching succeeds. Meaning, if we keep step 1), we have to keep
step 2) (to be sure).

So i would definitely keep them, as they are.
IF step 1) and the caching succeeds, we have a run time for installing the
dependencies in step 2) (for each parallel running job!) at about 8% (even with
multiple "npm install"s). If it doesn't succeed (haven't encountered that so far
for adblockpluschrome) we have a worst case scenario of 100% run time in *each*
subsequent job.
So the aforementioned only option would leave us with 100% run time for each
subsequent job, and mind that there are more to come (building / deploying)

[Sebastian Noack, 2018/08/30 15:17:28]

I agree that we should ideally have ensure_dependencies.py run before the jobs
run, so that we only download the dependencies once. But I'm a little surprised
that it's not guaranteed that the prepare stage completed / the cache exists
before running the individual jobs. Any chance that this was a configuration
issue, or bug in GitLab (that has been or will be fixed)? Can you actually
reproduce that behavior at the current time? If not, I might rather remove the
before_script step and revisit this once we run into it (if ever).

[tlucas, 2018/09/03 09:54:45]

As far as i understand (from their docs [1]) it's not a bug, but a limitation
and not subject to change (at least i found no reference) but very unlikely for
our setup (i.e. using the shell executor).
I personally could not reproduce that behavior. I would advice against removing
these steps from the jobs' before_script, but currently the automated workflow
seems to work without them. (keep in mind though, when a developer decides to
run a single job *only*, without the preparation - or a job from an earlier
commit, where the cache has been deleted in the mean time - the job will always
fail).
If you insist on removing them, please do say so and i will change it - however,
i would definitely advice against that move.

[1] https://docs.gitlab.com/ee/ci/caching/#availability-of-the-cache

[Sebastian Noack, 2018/09/03 19:37:35]

Fair enough.

     - npm install
-    - npm test
+  cache:
+    key: cache_$CI_COMMIT_SHA
+    paths:
+      - ./
+    policy: push
+
+
+.test_template: &test_template
+  stage: test
+  cache:
+    key: cache_$CI_COMMIT_SHA
+    paths:
+      - ./
+    policy: pull
+
+qunit:gecko:

[Sebastian Noack, 2018/08/28 11:22:55]

That the only thing currently performed is running the qunit tests is matter to
change. So I wouldn't name the job qunit:*, or do you plan to have a separate
job to evaluate the test pages? I might rather not, since then we have to
redundantly start the browser, initialize the driver, and install the extension.

Also we might want to have a separate job for ESlint. Otherwise, we'd
redundantly run ESLint for each job, and if it fails each job will fail.

[tlucas, 2018/08/28 12:21:59]

No plans for jobs per tests, so changed the naming to tests:*.

However, the latter has to be prepared in package.json (i.e. take "eslint" out
out "posttest").

[Sebastian Noack, 2018/08/28 12:50:57]

I know, go ahead. :)

[tlucas, 2018/08/29 08:14:27]

Done.

+  <<: *test_template
+  script:
+    - npm test -- -g gecko
+
+qunit:chrome:
+  <<: *test_template
+  script:
+    - xvfb-run npm test -- -g chrome