.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: Created Aug. 23, 2018, 7 p.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 - install

18 - 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.
19	18

20 .gecko_branch: &gecko_branch	19 qunit:gecko:
Sebastian Noack 2018/08/23 23:35:53 What does that do? What does that do? tlucas 2018/08/24 10:27:58 Removed now, but FWIW: this template (used as a v Show quoted text On 2018/08/23 23:35:53, Sebastian Noack wrote: > What does that do? Removed now, but FWIW: this template (used as a variable) was meant to keep the .yml dry (i.e. specify target branches for builds only once)
21 only:

22 - branches

23

24 prepare-dependencies:

25 stage: install

26 script:

27 - mkdir -p .git/info && touch .git/info/exclude
Sebastian Noack 2018/08/23 23:35:53 Perhaps we should rather adapt ensure_dependencies Perhaps we should rather adapt ensure_dependencies.py so that it doesn't attempt to setup the excludes if running from outside a repository. This would not only make this line redundant but also allows to run ensure_dependecies.py in other contexts (e.g. after unpacking the code from an archive file). tlucas 2018/08/24 10:27:58 While you're right about the perks, i'd rather not Show quoted text On 2018/08/23 23:35:53, Sebastian Noack wrote: > Perhaps we should rather adapt ensure_dependencies.py so that it doesn't attempt > to setup the excludes if running from outside a repository. This would not only > make this line redundant but also allows to run ensure_dependecies.py in other > contexts (e.g. after unpacking the code from an archive file). While you're right about the perks, i'd rather not initiate a change to the buildtools and a subsequent dependency update, when the only additional perk is nothing we desperately need / is not part of the prio directly. (FWIW the folder in question actually is a repository, it's just that .git/info is not created by the runners "git clone"/"git fetch" configuration)
28 - pip install --user -r build_requirements.txt
Sebastian Noack 2018/08/23 23:45:06 If the only purpose of the requirements file is to If the only purpose of the requirements file is to refer to it here and as long as it only contains a couple packages, I might rather inline the packages here. tlucas 2018/08/24 10:27:58 Done. Show quoted text On 2018/08/23 23:45:06, Sebastian Noack wrote: > If the only purpose of the requirements file is to refer to it here and as long > as it only contains a couple packages, I might rather inline the packages here. Done.
29 - python ensure_dependencies.py

30 - npm install

31 cache:

32 key: cache_$CI_COMMIT_SHA
Sebastian Noack 2018/08/23 23:35:53 Is there any point in caching if the cache is inva Is there any point in caching if the cache is invalidated with every new commit anyway? tlucas 2018/08/24 10:27:58 Removed now, but FWIW: The cache is shared across Show quoted text On 2018/08/23 23:35:53, Sebastian Noack wrote: > Is there any point in caching if the cache is invalidated with every new commit > anyway? Removed now, but FWIW: The cache is shared across all the jobs inside a pipline with this key, meaning "ensure_dependencies.py" is only run once (best case, no guarantee) for the testing, building and deployment stages. Otherwise each job would freshly clone the repository and would have to execute the full cloning process from ensure_dependencies.py again.
33 paths:

34 - ./

35 policy: push

36

37 # reusable template for browser-test jobs

38 .browser_test_t: &browser_test

39 stage: test_ext	20 stage: test_ext

40 script:	21 script:

41 - npm test -- $TARGET	22 - mkdir -p .git/info

42 cache:	23 - pip install --user Jinja2 cryptography
	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.
43 key: cache_$CI_COMMIT_SHA	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".
44 paths:	25 - npm test

45 - ./

46 policy: pull

47 environment:

48 name: devbuilds

49

50 # Invoke browser tests

51

52 qunit:gecko:

53 variables:

54 TARGET: gecko
Sebastian Noack 2018/08/23 23:35:53 Where is this variable picked up? Where is this variable picked up? tlucas 2018/08/24 10:27:58 Line 41, However, it's removed now. Show quoted text On 2018/08/23 23:35:53, Sebastian Noack wrote: > Where is this variable picked up? Line 41, However, it's removed now.
55 <<: *browser_test

56 <<: *gecko_branch

LEFT	RIGHT