Issue 29862580: Issue 6651 - Pt1: Qunit tests through gitlab CI (PENDING) (Closed)
|
Created:
Aug. 23, 2018, 7 p.m. by tlucas Modified:
Sept. 6, 2018, 5:30 p.m. CC:
kzar, sergei Base URL:
https://codereview.adblockplus.org/29860555/ Visibility:
Public. |
DescriptionIssue 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
MessagesTotal messages: 13
Patch Set 1: Issue 6651 - Pt1: Qunit tests through gitlab CI
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#newc... .gitlab-ci.yml:20: .gecko_branch: &gecko_branch What does that do? https://codereview.adblockplus.org/29862580/diff/29862581/.gitlab-ci.yml#newc... .gitlab-ci.yml:27: - mkdir -p .git/info && touch .git/info/exclude 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). https://codereview.adblockplus.org/29862580/diff/29862581/.gitlab-ci.yml#newc... .gitlab-ci.yml:32: key: cache_$CI_COMMIT_SHA Is there any point in caching if the cache is invalidated with every new commit anyway? https://codereview.adblockplus.org/29862580/diff/29862581/.gitlab-ci.yml#newc... .gitlab-ci.yml:54: TARGET: gecko Where is this variable picked up?
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#newc... .gitlab-ci.yml:28: - pip install --user -r build_requirements.txt 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. https://codereview.adblockplus.org/29862580/diff/29862581/build_requirements.txt File build_requirements.txt (right): https://codereview.adblockplus.org/29862580/diff/29862581/build_requirements.... build_requirements.txt:1: cryptography==2.2.2 I wonder whether we want to pin these versions. Against pinning exact versions arguments would be potential security fixes we will miss, additional maintainence effort if we start leveraging features of newer versions in the future, and that we don't pin exact versions for node modules either.
Patch Set 2: * Removed (currently) obsolete code (all those was only needed for more stages than "testing" in my case_study" 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#newc... .gitlab-ci.yml:20: .gecko_branch: &gecko_branch 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) https://codereview.adblockplus.org/29862580/diff/29862581/.gitlab-ci.yml#newc... .gitlab-ci.yml:27: - mkdir -p .git/info && touch .git/info/exclude 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) https://codereview.adblockplus.org/29862580/diff/29862581/.gitlab-ci.yml#newc... .gitlab-ci.yml:28: - pip install --user -r build_requirements.txt 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. https://codereview.adblockplus.org/29862580/diff/29862581/.gitlab-ci.yml#newc... .gitlab-ci.yml:32: key: cache_$CI_COMMIT_SHA 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. https://codereview.adblockplus.org/29862580/diff/29862581/.gitlab-ci.yml#newc... .gitlab-ci.yml:54: TARGET: gecko On 2018/08/23 23:35:53, Sebastian Noack wrote: > Where is this variable picked up? Line 41, However, it's removed now.
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): https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml#newc... .gitlab-ci.yml:19: qunit:gecko: 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!? https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml#newc... .gitlab-ci.yml:22: - mkdir -p .git/info && touch .git/info/exclude 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? https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml#newc... .gitlab-ci.yml:23: - pip install --user Jinja2 cryptography NIT: Depending on how the runner is setup it might make sense to tie down the exact dependencies!?
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#newc... .gitlab-ci.yml:19: qunit:gecko: 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. https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml#newc... .gitlab-ci.yml:22: - mkdir -p .git/info && touch .git/info/exclude 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 https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml#newc... .gitlab-ci.yml:23: - pip install --user Jinja2 cryptography 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....
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#newc... .gitlab-ci.yml:19: qunit:gecko: 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. https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml#newc... .gitlab-ci.yml:22: - mkdir -p .git/info && touch .git/info/exclude 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 https://codereview.adblockplus.org/29862580/diff/29863560/.gitlab-ci.yml#newc... .gitlab-ci.yml:23: - pip install --user Jinja2 cryptography 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.
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#newc... .gitlab-ci.yml:22: - mkdir -p .git/info && touch .git/info/exclude 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.
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#newc... .gitlab-ci.yml:22: - mkdir -p .git/info && touch .git/info/exclude 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.)
LGTM
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:24: - npm install what about moving all these steps but `npm test` into before_script?
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:24: - npm install 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.
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". 
|
