Refactor GHA build steps (#1446)

ssbarnea · web-flow · commit 6a3beeeb6359 · 2024-07-31T15:58:06.000+01:00
- simplify taskfile test commands
- sort GHA runner jobs for easier reading
- reduce complexity of GHA steps
- keep test steps separated on GHA to ease debugging failures
- have a single codecov action upload per job
- improve console logging during als testing
diff --git a/.c8rc b/.c8rc
@@ -1,6 +1,6 @@
 {
     "reporter": ["text", "html", "lcov"],
-    "report-dir": "./out/coverage/ui-test",
+    "report-dir": "./out/coverage/ui",
     "exclude": [
         "coverage/**",
         "packages/*/test{,s}/**",
diff --git a/.config/Containerfile b/.config/Containerfile
@@ -1,4 +1,4 @@
-FROM ghcr.io/ansible/creator-ee:v0.22.0 as DEFAULT_EE
+FROM ghcr.io/ansible/creator-ee:v24.2.0 as DEFAULT_EE
 # This file is updated by dependabot and used to determine not only which
 # version of creator-ee we are supposed to use for testing execution
 # environments but also to dynamically retrieve the same set of constraints
diff --git a/.config/dictionary.txt b/.config/dictionary.txt
@@ -158,6 +158,7 @@ neovim
 netcommon
 nocolor
 noheading
+norecursedirs
 notest
 notfound
 npmjs
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
@@ -21,24 +21,45 @@ env:
   PRETTIER_LEGACY_CLI: "1" # https://github.com/prettier/prettier/issues/15832
   # https://docs.github.com/en/actions/learn-github-actions/environment-variables
   # https://devblogs.microsoft.com/commandline/share-environment-vars-between-wsl-and-windows/
-  WSLENV: HOSTNAME:CI:FORCE_COLOR:GITHUB_ACTION:GITHUB_ACTION_PATH/p:GITHUB_ACTION_REPOSITORY:GITHUB_WORKFLOW:GITHUB_WORKSPACE/p:GITHUB_PATH/p:GITHUB_ENV/p:VIRTUAL_ENV/p
+  WSLENV: HOSTNAME:CI:FORCE_COLOR:GITHUB_ACTION:GITHUB_ACTION_PATH/p:GITHUB_ACTION_REPOSITORY:GITHUB_WORKFLOW:GITHUB_WORKSPACE/p:GITHUB_PATH/p:GITHUB_ENV/p:VIRTUAL_ENV/p:SKIP_PODMAN:SKIP_DOCKER
   # We define a hostname because otherwise the variable might not always be accessible on runners.
   HOSTNAME: gha
 
 # A workflow run is made up of one or more jobs that can run sequentially or in parallel
 jobs:
   build:
     name: ${{ matrix.name }}
-    environment: ci # codecov needed
+    environment: ci
     env:
-      SKIP_PODMAN: ${{ matrix.env.SKIP_PODMAN || 0 }}
       SKIP_DOCKER: ${{ matrix.env.SKIP_DOCKER || 0 }}
+      SKIP_PODMAN: ${{ matrix.env.SKIP_PODMAN || 0 }}
+      TASKFILE_ARGS: --output=group --output-group-begin='::group::{{.TASK}}' --output-group-end='::endgroup::'
+      # For using the mock Lightspeed server.
+      TEST_LIGHTSPEED_ACCESS_TOKEN: "dummy"
+      # The mock server runs on localhost. However, just using "localhost" as the hostname causes a few issues in
+      # GitHub Actions environment:
+      #
+      # On Linux: When "localhost" is used as the hostname, the mock server uses the ipv6 loopback address [::1] . However,
+      # the axios library, which is used in the extension tries to access to the ipv4 loopback 127.0.0.1 when "localhost"
+      # is specified and the axios library does not support URLs that contains ipv6 addresses, e.g. http://[::1]:3000.
+      # Also, If 127.0.0.1 is specified for the mock server, the server fails to start. Those issues are resolved by
+      # using the special ipv6-only hostname "ip6-localhost", which is available in GitHub Actions Linux environment.
+      #
+      # On MacOS: The hostname "ip6-localhost" is not available. However, 127.0.0.1 can be used for starting the mock
+      # server on MacOS and the axios library can connect to that address. So we can use 127.0.0.1 for MacOS.
+      #
+      # Once the axios library starts supporting URLs that contain ipv6 addresses, we will be able to use
+      # http://[::1]:3000 both on Linux and MacOS to get rid of the following conditional statement.
+      TEST_LIGHTSPEED_URL: "${{ contains(matrix.name, 'macos') && 'http://127.0.0.1:3000' || 'http://ip6-localhost:3000' }}"
+
+      # Set environment variables using matrix properties.
+      # For using an actual Lightspeed server instance, uncomment following two lines.
+      # TEST_LIGHTSPEED_ACCESS_TOKEN: ${{ secrets.TEST_LIGHTSPEED_ACCESS_TOKEN }}
+      # TEST_LIGHTSPEED_URL: ${{ secrets.TEST_LIGHTSPEED_URL }}
+
     defaults:
       run:
         shell: ${{ matrix.shell || 'bash'}}
-    permissions:
-      id-token: write
-      checks: read
     # The type of runner that the job will run on
     runs-on: ${{ matrix.os || 'ubuntu-22.04' }}
     # see https://github.com/containers/podman/issues/13609
@@ -50,32 +71,30 @@ jobs:
       matrix:
         # Avoid letting github do the matrix multiplication and use manual
         # includes for each job, this gives us fine control over job name.
+        # Order is important, keep it alphabetical: docs, lint, test*
         continue-on-error:
           - false
         os:
           - ubuntu-22.04
         task-name:
-          - test
+          - docs
         name:
-          - test
+          - docs
         include:
-          # - name: test-node-lts
-          #   task-name: test
-          #   task-name-als: als:test-node-lts
-          - name: docs
-            task-name: docs
-            task-name-als: ""
           - name: lint
             task-name: lint
-            task-name-als: ""
             os: ubuntu-22.04
             env:
               SKIP_PODMAN: 1
               SKIP_DOCKER: 1
 
+          - name: test (linux)
+            task-name: test
+            # https://github.com/ansible/vscode-ansible/issues/1473
+            ignore-als-test-failure: true
+
           - name: test (macos)
             task-name: test
-            task-name-als: als:test-without-ee
             os: macos-13-large
             env:
               SKIP_PODMAN: 1
@@ -84,12 +103,12 @@ jobs:
             # in order to enable the caching
             continue-on-error: true
 
-          - name: als:test-without-ee (wsl)
-            # runner does not support running container
+          - name: test (wsl)
+            # runner does not support running containers
             task-name: als:test-without-ee
             log-name: als-test-without-ee
             # https://github.com/actions/virtual-environments/issues/5151
-            os: windows-2022
+            os: devtools-win-x64
             shell: "wsl-bash {0}"
             env:
               SKIP_PODMAN: 1
@@ -223,87 +242,31 @@ jobs:
       #   uses: mxschmitt/action-tmate@v3
       - name: task package
         id: package
-        run: task package --output=group --output-group-begin='::group::{{.TASK}}' --output-group-end='::endgroup::'
+        run: task package ${{ matrix.env.TASKFILE_ARGS }}
 
       - name: task ${{ matrix.task-name }}
-        env:
-          # For using the mock Lightspeed server.
-          TEST_LIGHTSPEED_ACCESS_TOKEN: "dummy"
-          # The mock server runs on localhost. However, just using "localhost" as the hostname causes a few issues in
-          # GitHub Actions environment:
-          #
-          # On Linux: When "localhost" is used as the hostname, the mock server uses the ipv6 loopback address [::1] . However,
-          # the axios library, which is used in the extension tries to access to the ipv4 loopback 127.0.0.1 when "localhost"
-          # is specified and the axios library does not support URLs that contains ipv6 addresses, e.g. http://[::1]:3000.
-          # Also, If 127.0.0.1 is specified for the mock server, the server fails to start. Those issues are resolved by
-          # using the special ipv6-only hostname "ip6-localhost", which is available in GitHub Actions Linux environment.
-          #
-          # On MacOS: The hostname "ip6-localhost" is not available. However, 127.0.0.1 can be used for starting the mock
-          # server on MacOS and the axios library can connect to that address. So we can use 127.0.0.1 for MacOS.
-          #
-          # Once the axios library starts supporting URLs that contain ipv6 addresses, we will be able to use
-          # http://[::1]:3000 both on Linux and MacOS to get rid of the following conditional statement.
-          TEST_LIGHTSPEED_URL: "${{ contains(matrix.name, 'macos') && 'http://127.0.0.1:3000' || 'http://ip6-localhost:3000' }}"
-
-          # Set environment variables using matrix properties.
-          SKIP_PODMAN: "${{ matrix.env.SKIP_PODMAN || '0' }}"
-          SKIP_DOCKER: "${{ matrix.env.SKIP_DOCKER || '0' }}"
-
-          # For using an actual Lightspeed server instance, uncomment following two lines.
-          # TEST_LIGHTSPEED_ACCESS_TOKEN: ${{ secrets.TEST_LIGHTSPEED_ACCESS_TOKEN }}
-          # TEST_LIGHTSPEED_URL: ${{ secrets.TEST_LIGHTSPEED_URL }}
-        run: task ${{ matrix.task-name }} --output=group --output-group-begin='::group::{{.TASK}}' --output-group-end='::endgroup::'
-
-      - name: task ${{ matrix.task-name-als }}
-        if: ${{ (matrix.task-name-als || '') != '' }}
-        env:
-          SKIP_PODMAN: "${{ matrix.env.SKIP_PODMAN || '0' }}"
-          SKIP_DOCKER: "${{ matrix.env.SKIP_DOCKER || '0' }}"
-        run: task ${{ matrix.task-name-als }} --output=group --output-group-begin='::group::{{.TASK}}' --output-group-end='::endgroup::'
+        if: "${{ !contains(matrix.name, 'test') }}"
+        run: task ${{ matrix.task-name }} ${{ matrix.env.TASKFILE_ARGS }}
 
-      - name: Upload coverage data (unit test)
-        if: ${{ startsWith(matrix.name, 'test') }}
-        uses: codecov/codecov-action@v4
-        with:
-          name: ${{ matrix.name }}
-          files: out/coverage/unit/lcov.info
-          disable_search: true
-          flags: unit
-          fail_ci_if_error: true
-          use_oidc: true # cspell:ignore oidc
+      - name: task unit
+        if: contains(matrix.name, 'test')
+        run: task unit ${{ matrix.env.TASKFILE_ARGS }}
 
-      - name: Upload coverage data (ui test)
-        if: ${{ startsWith(matrix.name, 'test') }}
-        uses: codecov/codecov-action@v4
-        with:
-          name: ${{ matrix.name }}
-          files: out/coverage/ui-test/lcov.info
-          disable_search: true
-          flags: ui
-          fail_ci_if_error: true
-          use_oidc: true # cspell:ignore oidc
+      - name: task ui
+        # https://github.com/ansible/vscode-ansible/issues/1451
+        if: "${{ contains(matrix.name, 'test') && !contains(matrix.name, 'wsl') }}"
+        run: task ui ${{ matrix.env.TASKFILE_ARGS }}
 
-      - name: Upload coverage data (e2e test)
-        if: ${{ startsWith(matrix.name, 'test') }}
-        uses: codecov/codecov-action@v4
-        with:
-          name: ${{ matrix.name }}
-          files: out/coverage/e2e/lcov.info
-          disable_search: true
-          flags: e2e
-          fail_ci_if_error: true
-          use_oidc: true # cspell:ignore oidc
+      - name: task e2e
+        # https://github.com/ansible/vscode-ansible/issues/1451
+        if: "${{ contains(matrix.name, 'test') && !contains(matrix.name, 'wsl') }}"
+        run: task e2e ${{ matrix.env.TASKFILE_ARGS }}
 
-      - name: Upload coverage data (als)
-        if: contains(matrix.task-name-als, 'test') || contains(matrix.task-name, 'als:test')
-        uses: codecov/codecov-action@v4
-        with:
-          name: ${{ matrix.name }}
-          files: out/coverage/als/lcov.info
-          disable_search: true
-          flags: als
-          fail_ci_if_error: true
-          use_oidc: true # cspell:ignore oidc
+      - name: task als
+        # https://github.com/ansible/vscode-ansible/issues/1451
+        if: contains(matrix.name, 'test')
+        continue-on-error: ${{ matrix.ignore-als-test-failure || false }}
+        run: task als ${{ matrix.env.TASKFILE_ARGS }}
 
       - name: Upload vsix artifact
         if: ${{ matrix.name == 'test' }}
@@ -323,12 +286,13 @@ jobs:
           if-no-files-found: error
           retention-days: 90
 
-      - name: Upload test logs
+      - name: Upload test logs and reports
         if: ${{ always() }}
         uses: actions/upload-artifact@v4
         with:
           name: logs-${{ matrix.os }}-${{  matrix.log-name || matrix.task-name }}.zip
           path: |
+            out/coverage
             out/e2eTestReport
             out/log
             out/test-resources/settings/logs
@@ -367,6 +331,10 @@ jobs:
     needs:
       - build
 
+    permissions: # codecov
+      id-token: write
+      checks: read
+
     runs-on: ubuntu-22.04
 
     steps:
@@ -375,8 +343,55 @@ jobs:
         with:
           name: logs.zip
           pattern: logs*.zip
+          separate-directories: true
           delete-merged: true
 
+      - name: Download artifacts
+        uses: actions/download-artifact@v4
+        with:
+          name: logs.zip
+          path: .
+
+      - name: Upload als test coverage data [1/4]
+        uses: codecov/codecov-action@v4
+        with:
+          name: als
+          files: ./*/coverage/als/lcov.info
+          flags: als
+          disable_search: true
+          fail_ci_if_error: true
+          use_oidc: true # cspell:ignore oidc
+
+      - name: Upload unit test coverage data [2/4]
+        uses: codecov/codecov-action@v4
+        with:
+          name: unit
+          files: ./*/coverage/unit/lcov.info
+          flags: unit
+          disable_search: true
+          fail_ci_if_error: true
+          use_oidc: true # cspell:ignore oidc
+
+      - name: Upload ui test coverage data [3/4]
+        uses: codecov/codecov-action@v4
+        with:
+          name: unit
+          files: ./*/coverage/ui/lcov.info
+          flags: ui
+          disable_search: true
+          fail_ci_if_error: true
+          use_oidc: true # cspell:ignore oidc
+
+      - name: Upload e2e test coverage data [4/4]
+        uses: codecov/codecov-action@v4
+        with:
+          name: e2e
+          files: ./*/coverage/e2e/lcov.info
+          flags: e2e
+          disable_search: true
+          fail_ci_if_error: true
+          use_oidc: true # cspell:ignore oidc
+
       - name: Decide whether the needed jobs succeeded or failed
         uses: re-actors/alls-green@release/v1
         with:
diff --git a/Taskfile.yml b/Taskfile.yml
@@ -131,41 +131,38 @@ tasks:
       - test
       - tools
   test:
-    desc: Run all tests
+    # cspell: disable-next-line
+    desc: "Run all tests: \e[22;32munit, ui, e2e, als\e[0m"
     cmds:
       - task: setup
       - task: unit
-      - task: test-ui
-      #- task: test-ui-old
-      - task: test-e2e
+      - task: ui
+      - task: e2e
+      - task: als:test
       # .vsix file is no longer needed for executing tests.
       # This is just for making sure that the package step works w/o issues.
       - task: package
       - tools/dirty.sh
     interactive: true
   test-e2e:
     desc: Run e2e tests {{.XVFB}}
+    aliases: [e2e]
     cmds:
       - task: package
       - $VIRTUAL_ENV/bin/python3 --version
       - '{{.XVFB}} bash -c ''source "${VIRTUAL_ENV}/bin/activate" && yarn run _coverage-all'''
     interactive: true
   test-ui:
     desc: Run UI tests {{.XVFB}}
+    aliases: [ui]
     cmds:
       - yarn webpack-dev
       - $VIRTUAL_ENV/bin/python3 --version
       - ' {{.XVFB}} bash -c ''source "${VIRTUAL_ENV}/bin/activate" && yarn _coverage-ui-with-mock-lightspeed-server'''
     interactive: true
-  # test-ui-old:
-  #   desc: Run UI tests (oldest vscode)
-  #   cmds:
-  #     - task: package
-  #     - $VIRTUAL_ENV/bin/python3 --version
-  #     - bash -c 'source "${VIRTUAL_ENV}/bin/activate" && yarn run test-ui-oldest'
-  #   interactive: true
-  unit:
+  test-unit:
     desc: Run unit tests with coverage report
+    aliases: [unit]
     cmds:
       - npm run unit-tests
     interactive: true
diff --git a/codecov.yml b/codecov.yml
@@ -1,5 +1,8 @@
 codecov:
   require_ci_to_pass: true
+  notify:
+    after_n_builds: 3 # effectively number of uploads: linux, macos, wsl
+    wait_for_ci: true
 comment: false
 coverage:
   status:
diff --git a/package.json b/package.json
@@ -990,7 +990,7 @@
     "package": "./tools/helper --package",
     "preinstall": "",
     "pretest": "yarn run compile",
-    "unit-tests": "nyc mocha --recursive test/units/lightspeed",
+    "unit-tests": "sh -c \"nyc mocha --recursive test/units/lightspeed ${MOCHA_OPTS:-}\"",
     "test-ui": "yarn run test-ui-current && yarn run test-ui-oldest",
     "_test-ui": "extest get-vscode -c ${CODE_VERSION} -s out/test-resources && extest get-chromedriver -c ${CODE_VERSION} -s out/test-resources && extest install-vsix -f ansible-*.vsix -e out/ext -s out/test-resources && extest install-from-marketplace redhat.vscode-yaml ms-python.python -e out/ext -s out/test-resources && extest run-tests -s out/test-resources -e out/ext --code_settings test/testFixtures/settings.json out/client/test/ui-test/allTestsSuite.js",
     "test-ui-current": "CODE_VERSION='max' yarn _test-ui",
diff --git a/packages/ansible-language-server/package.json b/packages/ansible-language-server/package.json
diff --git a/pyproject.toml b/pyproject.toml
diff --git a/test/testRunner.ts b/test/testRunner.ts

Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"reporter": ["text", "html", "lcov"],`
`3`		`- "report-dir": "./out/coverage/ui-test",`
	`3`	`+ "report-dir": "./out/coverage/ui",`
`4`	`4`	`"exclude": [`
`5`	`5`	`"coverage/**",`
`6`	`6`	`"packages//test{,s}/*",`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-FROM ghcr.io/ansible/creator-ee:v0.22.0 as DEFAULT_EE`
	`1`	`+FROM ghcr.io/ansible/creator-ee:v24.2.0 as DEFAULT_EE`
`2`	`2`	`# This file is updated by dependabot and used to determine not only which`
`3`	`3`	`# version of creator-ee we are supposed to use for testing execution`
`4`	`4`	`# environments but also to dynamically retrieve the same set of constraints`