Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(386)

Issue 29862580: Issue 6651 - Pt1: Qunit tests through gitlab CI (PENDING) (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 7 months ago by tlucas
Modified:
1 year, 7 months ago
Reviewers:
Sebastian Noack, wspee
CC:
kzar, sergei
Base URL:
https://codereview.adblockplus.org/29860555/
Visibility:
Public.

Description

Issue 6651 - Pt1: Qunit tests through gitlab CI Please note that this *only* can land, once our gitlab-runners are setup. Please note the Base URL.

Patch Set 1 #

Total comments: 11

Patch Set 2 : Rebased against 29860555@PS9 #

Total comments: 11

Patch Set 3 : #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -0 lines) Patch
A .gitlab-ci.yml View 1 2 1 chunk +25 lines, -0 lines 5 comments Download

Messages

Total messages: 13
tlucas
Patch Set 1: Issue 6651 - Pt1: Qunit tests through gitlab CI
1 year, 7 months ago (2018-08-23 19:02:18 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29862580/diff/29862581/.gitlab-ci.yml File .gitlab-ci.yml (right): https://codereview.adblockplus.org/29862580/diff/29862581/.gitlab-ci.yml#newcode20 .gitlab-ci.yml:20: .gecko_branch: &gecko_branch What does that do? https://codereview.adblockplus.org/29862580/diff/29862581/.gitlab-ci.yml#newcode27 .gitlab-ci.yml:27: - ...
1 year, 7 months ago (2018-08-23 23:35:54 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29862580/diff/29862581/.gitlab-ci.yml File .gitlab-ci.yml (right): https://codereview.adblockplus.org/29862580/diff/29862581/.gitlab-ci.yml#newcode28 .gitlab-ci.yml:28: - pip install --user -r build_requirements.txt If the only ...
1 year, 7 months ago (2018-08-23 23:45:07 UTC) #3
tlucas
Patch Set 2: * Removed (currently) obsolete code (all those was only needed for more ...
1 year, 7 months ago (2018-08-24 10:27:58 UTC) #4
wspee
Left a few question mainly for myself, apart from that LGTM. https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml File .gitlab-ci.yml (right): ...
1 year, 7 months ago (2018-08-24 11:32:32 UTC) #5
tlucas
https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml File .gitlab-ci.yml (right): https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml#newcode19 .gitlab-ci.yml:19: qunit:gecko: On 2018/08/24 11:32:32, wspee wrote: > FMI: Has ...
1 year, 7 months ago (2018-08-24 12:17:38 UTC) #6
wspee
Still LGTM https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml File .gitlab-ci.yml (right): https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml#newcode19 .gitlab-ci.yml:19: qunit:gecko: On 2018/08/24 12:17:38, tlucas wrote: > ...
1 year, 7 months ago (2018-08-24 12:38:56 UTC) #7
Sebastian Noack
https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml File .gitlab-ci.yml (right): https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml#newcode22 .gitlab-ci.yml:22: - mkdir -p .git/info && touch .git/info/exclude On 2018/08/24 ...
1 year, 7 months ago (2018-08-24 12:49:19 UTC) #8
tlucas
Patch Set 3: * let ensure_dependencies.py create "exclude" when necessary https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml File .gitlab-ci.yml (right): https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml#newcode22 ...
1 year, 7 months ago (2018-08-24 13:04:01 UTC) #9
Sebastian Noack
LGTM
1 year, 7 months ago (2018-08-24 13:08:20 UTC) #10
sergei
https://codereview.adblockplus.org/29862580/diff/29863567/.gitlab-ci.yml File .gitlab-ci.yml (right): https://codereview.adblockplus.org/29862580/diff/29863567/.gitlab-ci.yml#newcode24 .gitlab-ci.yml:24: - npm install what about moving all these steps ...
1 year, 7 months ago (2018-08-24 13:19:24 UTC) #11
tlucas
https://codereview.adblockplus.org/29862580/diff/29863567/.gitlab-ci.yml File .gitlab-ci.yml (right): https://codereview.adblockplus.org/29862580/diff/29863567/.gitlab-ci.yml#newcode24 .gitlab-ci.yml:24: - npm install On 2018/08/24 13:19:23, sergei wrote: > ...
1 year, 7 months ago (2018-08-24 13:35:23 UTC) #12
sergei
1 year, 7 months ago (2018-08-24 14:20:08 UTC) #13
https://codereview.adblockplus.org/29862580/diff/29863567/.gitlab-ci.yml
File .gitlab-ci.yml (right):

https://codereview.adblockplus.org/29862580/diff/29863567/.gitlab-ci.yml#newc...
.gitlab-ci.yml:17: - test_ext
I'm also not sure that it's already time to introduce stages, but it's fine for
me.

https://codereview.adblockplus.org/29862580/diff/29863567/.gitlab-ci.yml#newc...
.gitlab-ci.yml:23: - pip install --user Jinja2 cryptography
I would also think about adding of `pip install -U pip`. According to my
experience, images of CI sometimes are not updated so often and as the result
one gets complains about obsolete pip.

https://codereview.adblockplus.org/29862580/diff/29863567/.gitlab-ci.yml#newc...
.gitlab-ci.yml:24: - npm install
On 2018/08/24 13:35:22, tlucas wrote:
> On 2018/08/24 13:19:23, sergei wrote:
> > what about moving all these steps but `npm test` into before_script?
> 
> By the time we introduce more stages (building / deploying) to the CI/CD,
those
> would be run for *every* job and severely slow down execution time. The
solution
> for that is caching, so those steps will be removed from before_script later
on
> anyway.

I personally don't find caching of such things as a good idea, but it's up to
any project team. Nevertheless, I proposed to create before_script for this
particular job mainly for the sake of order and self-description. All these
steps of preparing the environment should be separated from the actual actions,
in this case from running the tests. I don't insist on it, it's just a good
practice which is used in other projects, despite the everything could be in
`script`.

Do I correctly understand that a global before_script is executed for each job,
thus several times? If so, then I think that one could add a stage "prepare" and
just move the content of the current `before_script` to `script` of "prepare".
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5