.gitlab-ci.yml - Issue 29862580: Issue 6651 - Pt1: Qunit tests through gitlab CI (PENDING)

Delta Between Two Patch Sets: .gitlab-ci.yml

Issue 29862580: Issue 6651 - Pt1: Qunit tests through gitlab CI (PENDING) (Closed) Base URL: https://codereview.adblockplus.org/29860555/

Left Patch Set: Rebased against 29860555@PS9 Created Aug. 24, 2018, 10:26 a.m.

Right Patch Set: Created Aug. 24, 2018, 1:03 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 # This file is part of Adblock Plus <https://adblockplus.org/>,	1 # This file is part of Adblock Plus <https://adblockplus.org/>,

2 # Copyright (C) 2006-present eyeo GmbH	2 # Copyright (C) 2006-present eyeo GmbH

3 #	3 #

4 # Adblock Plus is free software: you can redistribute it and/or modify	4 # Adblock Plus is free software: you can redistribute it and/or modify

5 # it under the terms of the GNU General Public License version 3 as	5 # it under the terms of the GNU General Public License version 3 as

6 # published by the Free Software Foundation.	6 # published by the Free Software Foundation.

7 #	7 #

8 # Adblock Plus is distributed in the hope that it will be useful,	8 # Adblock Plus is distributed in the hope that it will be useful,

9 # but WITHOUT ANY WARRANTY; without even the implied warranty of	9 # but WITHOUT ANY WARRANTY; without even the implied warranty of

10 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the	10 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

11 # GNU General Public License for more details.	11 # GNU General Public License for more details.

12 #	12 #

13 # You should have received a copy of the GNU General Public License	13 # You should have received a copy of the GNU General Public License

14 # along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.	14 # along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.

15	15

16 stages:	16 stages:

17 - test_ext	17 - test_ext
	sergei 2018/08/24 14:20:07 I'm also not sure that it's already time to introd I'm also not sure that it's already time to introduce stages, but it's fine for me.
18	18

19 qunit:gecko:	19 qunit:gecko:
wspee 2018/08/24 11:32:32 FMI: Has the colon between qunit and gecko any eff FMI: Has the colon between qunit and gecko any effect or is it just part of the job name? I'm asking because in the context of YAML this might be confused as an associative arrays!? tlucas 2018/08/24 12:17:38 It's only some sugar for the naming. It might be c Show quoted text On 2018/08/24 11:32:32, wspee wrote: > FMI: Has the colon between qunit and gecko any effect or is it just part of the > job name? I'm asking because in the context of YAML this might be confused as an > associative arrays!? It's only some sugar for the naming. It might be confused with a mapping, yes, but FWIW a mapping in YAML is always a colon, followed by at least one space. wspee 2018/08/24 12:38:56 Acknowledged. Show quoted text On 2018/08/24 12:17:38, tlucas wrote: > On 2018/08/24 11:32:32, wspee wrote: > > FMI: Has the colon between qunit and gecko any effect or is it just part of > the > > job name? I'm asking because in the context of YAML this might be confused as > an > > associative arrays!? > > It's only some sugar for the naming. It might be confused with a mapping, yes, > but FWIW a mapping in YAML is always a colon, followed by at least one space. Acknowledged.
20 stage: test_ext	20 stage: test_ext

21 script:	21 script:

22 - mkdir -p .git/info && touch .git/info/exclude	22 - mkdir -p .git/info
wspee 2018/08/24 11:32:32 Out of curiosity: Do you know why .git/info is mis Out of curiosity: Do you know why .git/info is missing? I wasn't able to reproduce this locally and wasn't able to find any information only? tlucas 2018/08/24 12:17:38 This happens because the gitlab-runner overwrites Show quoted text On 2018/08/24 11:32:32, wspee wrote: > Out of curiosity: Do you know why .git/info is missing? I wasn't able to > reproduce this locally and wasn't able to find any information only? This happens because the gitlab-runner overwrites the default git-template-directory (where .git/info etc would be defined) with a custom, temporary template-directory: https://gitlab.com/gitlab-org/gitlab-runner/blob/master/shells/abstract.go#L63 This was introduced in: https://gitlab.com/gitlab-org/gitlab-runner/merge_requests/314 wspee 2018/08/24 12:38:56 Cool, thanks +1 Show quoted text On 2018/08/24 12:17:38, tlucas wrote: > On 2018/08/24 11:32:32, wspee wrote: > > Out of curiosity: Do you know why .git/info is missing? I wasn't able to > > reproduce this locally and wasn't able to find any information only? > > This happens because the gitlab-runner overwrites the default > git-template-directory (where .git/info etc would be defined) with a custom, > temporary template-directory: > > https://gitlab.com/gitlab-org/gitlab-runner/blob/master/shells/abstract.go#L63 > > This was introduced in: > https://gitlab.com/gitlab-org/gitlab-runner/merge_requests/314 Cool, thanks +1 Sebastian Noack 2018/08/24 12:49:18 At least creating .git/info/exclude seems redundan Show quoted text On 2018/08/24 12:17:38, tlucas wrote: > On 2018/08/24 11:32:32, wspee wrote: > > Out of curiosity: Do you know why .git/info is missing? I wasn't able to > > reproduce this locally and wasn't able to find any information only? > > This happens because the gitlab-runner overwrites the default > git-template-directory (where .git/info etc would be defined) with a custom, > temporary template-directory: > > https://gitlab.com/gitlab-org/gitlab-runner/blob/master/shells/abstract.go#L63 > > This was introduced in: > https://gitlab.com/gitlab-org/gitlab-runner/merge_requests/314 At least creating .git/info/exclude seems redundant. It seems as long as the parent directory exists, ensure_dependencies.py will just create the file if it doesn't exist. The -p flag is probably unnecessary too, but I guess, you could argue it's more safe just in case. tlucas 2018/08/24 13:04:01 You are right about .git/info/exclude, removed tha Show quoted text On 2018/08/24 12:49:18, Sebastian Noack wrote: > On 2018/08/24 12:17:38, tlucas wrote: > > On 2018/08/24 11:32:32, wspee wrote: > > > Out of curiosity: Do you know why .git/info is missing? I wasn't able to > > > reproduce this locally and wasn't able to find any information only? > > > > This happens because the gitlab-runner overwrites the default > > git-template-directory (where .git/info etc would be defined) with a custom, > > temporary template-directory: > > > > https://gitlab.com/gitlab-org/gitlab-runner/blob/master/shells/abstract.go#L63 > > > > This was introduced in: > > https://gitlab.com/gitlab-org/gitlab-runner/merge_requests/314 > > At least creating .git/info/exclude seems redundant. It seems as long as the > parent directory exists, ensure_dependencies.py will just create the file if it > doesn't exist. The -p flag is probably unnecessary too, but I guess, you could > argue it's more safe just in case. You are right about .git/info/exclude, removed that one. However, the -p is mandatory for handling both scenarios, where ".git" is not yet created or already exists (the latter is an edge case that might occur when the git-strategy for getting the changes is "fetch" instead of "clone" and e.g. when you manually rerun a job for an already built commit.)
23 - pip install --user Jinja2 cryptography	23 - pip install --user Jinja2 cryptography
wspee 2018/08/24 11:32:32 NIT: Depending on how the runner is setup it might NIT: Depending on how the runner is setup it might make sense to tie down the exact dependencies!? tlucas 2018/08/24 12:17:38 See this comment on the previous PS: https://coder Show quoted text On 2018/08/24 11:32:32, wspee wrote: > NIT: Depending on how the runner is setup it might make sense to tie down the > exact dependencies!? See this comment on the previous PS: https://codereview.adblockplus.org/29862580/diff/29862581/build_requirements.... wspee 2018/08/24 12:38:56 Acknowledged. Show quoted text On 2018/08/24 12:17:38, tlucas wrote: > On 2018/08/24 11:32:32, wspee wrote: > > NIT: Depending on how the runner is setup it might make sense to tie down the > > exact dependencies!? > > See this comment on the previous PS: > https://codereview.adblockplus.org/29862580/diff/29862581/build_requirements.... Acknowledged.	sergei 2018/08/24 14:20:07 I would also think about adding of `pip install -U 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.
24 - npm install	24 - npm install
	sergei 2018/08/24 13:19:23 what about moving all these steps but `npm test` i what about moving all these steps but `npm test` into before_script? tlucas 2018/08/24 13:35:22 By the time we introduce more stages (building / d Show quoted text 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. sergei 2018/08/24 14:20:08 I personally don't find caching of such things as Show quoted text 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".
25 - npm test	25 - npm test

LEFT	RIGHT