Re: [Tarantool-patches] [PATCH luajit] ci: extend tarantool integration testing

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!
Thanks for the patch!
Please consider my questions below.

On 17.02.24, Maxim Kokryashkin wrote:
> This patch makes use of reusable workflows from the
> Tarantool repository, so now LuaJIT CI tests integration
> in all relevant workflows.
> 
> Also, this patch disables MacOS testing for Tarantool
> integration since those runs were made nightly in the
> Tarantool repo.
> ---

I see that PR [1] isn't backported to release/2.11 and release/3.0
branches, so newly introduced workflow won't work on them after
backporting to the corresponding branches.

How should we track new workflows to be added in the Tarantool's repo?
Or if some workflow will be deleted (except a red CI of cause:))?

Do all workflows run LuaJIT-test before Tarantool tests?
Is there a way to skip LuaJIT tests for workflows that duplicate
luajit-integration.yml?

> Branch: https://github.com/tarantool/luajit/tree/fckxorg/integration-testing
>  .github/workflows/exotic-builds-testing.yml |  34 +++++
>  .github/workflows/tarantool-integration.yml |  35 ++++++
>  .github/workflows/testing.yml               | 130 +++++++++++++++-----
>  3 files changed, 169 insertions(+), 30 deletions(-)
>  create mode 100644 .github/workflows/tarantool-integration.yml
> 
> diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
> index a9ba5fd5..46a3c1ef 100644
> --- a/.github/workflows/exotic-builds-testing.yml
> +++ b/.github/workflows/exotic-builds-testing.yml
> @@ -82,3 +82,37 @@ jobs:
>        - name: test
>          run: cmake --build . --parallel --target LuaJIT-test
>          working-directory: ${{ env.BUILDDIR }}
> +

Exotic builds is about **LuaJIT** exotic build. If we want
exotic-tarantool integration or whatever we should use separate
workflow. Also, it may be introduced as a separate job in the
tarantool-integration workflow. Also, it should depends on
luajit-testing, so it still should be placed back in the <testing.yml>.

> +  test-exotic-tarantool:
> +    strategy:
> +      fail-fast: false
> +      matrix:
> +        ARCH: [ARM64, x86_64]
> +        BUILDTYPE: [Debug, Release]
> +        GC64: [ON, OFF]
> +        OS: [Linux, macOS]
> +        include:
> +          - BUILDTYPE: Debug
> +            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug
> +          - BUILDTYPE: Release
> +            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
> +        exclude:
> +          - ARCH: ARM64
> +            GC64: OFF
> +          - OS: macOS
> +            GC64: OFF
> +    name: >
> +      Tarantool
> +      (${{ matrix.OS }}/${{ matrix.ARCH }})
> +      ${{ matrix.BUILDTYPE }}
> +      GC64:${{ matrix.GC64 }}
> +    needs: test-exotic
> +    uses: tarantool/tarantool/.github/workflows/luajit-integration.yml@master

Please, add the comment that only LuaJIT-tests are running in this
workflow.

> +    with:
> +      CMAKE_EXTRA_PARAMS: >
> +        -G Ninja
> +        ${{ matrix.CMAKEFLAGS }}
> +        -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
> +      arch: ${{ matrix.ARCH }}
> +      os: ${{ matrix.OS }}
> +      revision: ${{ github.sha }}
> diff --git a/.github/workflows/tarantool-integration.yml b/.github/workflows/tarantool-integration.yml
> new file mode 100644
> index 00000000..81abfb94
> --- /dev/null
> +++ b/.github/workflows/tarantool-integration.yml

The name of the workflow is a little bit misleading -- it is not
integration with Tarantool, but integrational testing of Tarantool with
connectors, etc.:).
Here comes the question: Why do we need to separate this job from
others? I suppose dependency on LuaJIT tests is good here and shouldn't
be omitted -- if LuaJIT tests fail, there is no reason to run
integrational tests of Tarantool with its ecosystem.

> @@ -0,0 +1,35 @@
> +name: Tarantool integration testing
> +
> +on:
> +  push:
> +    branches-ignore:
> +      - '**-notest'
> +      - 'upstream-**'
> +    tags-ignore:
> +      - '**'
> +
> +concurrency:

<snipped>

> +  cancel-in-progress: true
> +
> +jobs:
> +  test-tarantool-integration:
> +    uses: tarantool/tarantool/.github/workflows/integration.yml@master
> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}
> +
> diff --git a/.github/workflows/testing.yml b/.github/workflows/testing.yml
> index cb4ba57b..f9e38bf1 100644
> --- a/.github/workflows/testing.yml
> +++ b/.github/workflows/testing.yml
> @@ -83,37 +83,107 @@ jobs:
>          run: cmake --build . --parallel --target LuaJIT-test
>          working-directory: ${{ env.BUILDDIR }}
> 
> +  test-tarantool-coverage:

Please add a comment explaining why this job is included (IINM, it
rather long to show some corner cases of our tests for profilers).

> +    needs: test-luajit
> +    uses: tarantool/tarantool/.github/workflows/coverage.yml@master
> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}
> 
> -  test-tarantool:
> -    strategy:
> -      fail-fast: false
> -      matrix:
> -        ARCH: [ARM64, x86_64]
> -        BUILDTYPE: [Debug, Release]
> -        GC64: [ON, OFF]
> -        OS: [Linux, macOS]
> -        include:
> -          - BUILDTYPE: Debug
> -            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug
> -          - BUILDTYPE: Release
> -            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
> -        exclude:
> -          - ARCH: ARM64
> -            GC64: OFF
> -          - OS: macOS
> -            GC64: OFF
> -    name: >
> -      Tarantool
> -      (${{ matrix.OS }}/${{ matrix.ARCH }})
> -      ${{ matrix.BUILDTYPE }}
> -      GC64:${{ matrix.GC64 }}
> +  test-tarantool-debug:
> +    needs: test-luajit
> +    uses: tarantool/tarantool/.github/workflows/debug.yml@master
> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}

Can we use a matrix of jobs instead to avoid copy-pasting below?

> +
> +  test-tarantool-debug_aarch64:
> +    needs: test-luajit
> +    uses: tarantool/tarantool/.github/workflows/debug_aarch64.yml@master
> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}
> +
> +  test-tarantool-debug_asan_clang:
> +    needs: test-luajit
> +    uses: tarantool/tarantool/.github/workflows/debug_asan_clang.yml@master
> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}
> +
> +  test-tarantool-default_gcc_centos_7:
> +    needs: test-luajit
> +    uses: tarantool/tarantool/.github/workflows/default_gcc_centos_7.yml@master

Please add a comment about ancient compilers we want to check here.

> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}
> +
> +  test-tarantool-memtx_allocator_based_on_malloc:
> +    needs: test-luajit
> +    uses: tarantool/tarantool/.github/workflows/memtx_allocator_based_on_malloc.yml@master

Why do we need this workflow?

> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}
> +
> +  test-tarantool-out_of_source:
> +    needs: test-luajit
> +    uses: tarantool/tarantool/.github/workflows/out_of_source.yml@master
> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}
> +
> +  test-tarantool-perf_micro:
> +    needs: test-luajit
> +    uses: tarantool/tarantool/.github/workflows/perf_micro.yml@master

Why do we need this workflow? IINM, it includes only C perf tests that
unrelated to Lua world of Tarantool.

> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}
> +
> +  test-tarantool-release:
> +    needs: test-luajit
> +    uses: tarantool/tarantool/.github/workflows/release.yml@master
> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}
> +
> +  test-tarantool-release_asan_clang:
> +    needs: test-luajit
> +    uses: tarantool/tarantool/.github/workflows/release_asan_clang.yml@master
> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}
> +
> +  test-tarantool-release_clang:
> +    needs: test-luajit
> +    uses: tarantool/tarantool/.github/workflows/release_clang.yml@master
> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}
> +
> +  test-tarantool-release_lto:
> +    needs: test-luajit
> +    uses: tarantool/tarantool/.github/workflows/release_lto.yml@master
> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}
> +
> +  test-tarantool-release_lto_clang:
> +    needs: test-luajit
> +    uses: tarantool/tarantool/.github/workflows/release_lto_clang.yml@master
> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}
> +
> +  test-tarantool-static_build:
> +    needs: test-luajit
> +    uses: tarantool/tarantool/.github/workflows/static_build.yml@master
> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}
> +
> +  test-tarantool-static_build_cmake_linux:
>      needs: test-luajit
> -    uses: tarantool/tarantool/.github/workflows/luajit-integration.yml@master
> +    uses: tarantool/tarantool/.github/workflows/static_build_cmake_linux.yml@master
>      with:
> -      CMAKE_EXTRA_PARAMS: >
> -        -G Ninja
> -        ${{ matrix.CMAKEFLAGS }}
> -        -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
> -      arch: ${{ matrix.ARCH }}
> -      os: ${{ matrix.OS }}
> +      submodule: luajit
>        revision: ${{ github.sha }}

There is the list of all Tarantool workflows:
* codeql.yml
* coverage.yml - used
* coverity.yml
* debug.yml - used
* debug_aarch64.yml - used
* debug_asan_clang.yml - used
* default_gcc_centos_7.yml - used
* freebsd.yml
* fuzzing.yml
* integration.yml - used
* jepsen-cluster-txm.yml
* jepsen-cluster.yml
* jepsen-single-instance-txm.yml
* jepsen-single-instance.yml
* lango-stale-reviews.yml
* lint.yml
* luajit-integration.yml - used
* memtx_allocator_based_on_malloc.yml - used
* osx.yml
* out_of_source.yml
* packaging.yml
* perf_cbench.yml
* perf_linkbench_ssd.yml
* perf_micro.yml
* perf_nosqlbench_hash.yml
* perf_nosqlbench_tree.yml
* perf_sysbench.yml
* perf_tpcc.yml
* perf_tpch.yml
* perf_ycsb_hash.yml
* perf_ycsb_tree.yml
* publish-module-api-doc.yaml
* release.yml - used
* release_asan_clang.yml - used
* release_clang.yml - used
* release_lto.yml - used
* release_lto_clang.yml - used
* reusable_build.yml
* source.yml
* static_build.yml - used
* static_build_cmake_linux.yml - used
* static_build_pack_test_deploy.yml
* submodule_update.yml

As you can see, LuaJIT-integration is used only 13/45. It will be nice
to have the table in the commit message with the following format:
workflow name (wo .yml) | used (+/-) | Reason

> --
> 2.43.0
> 

[1]: https://github.com/tarantool/tarantool/pull/9560

-- 
Best regards,
Sergey Kaplun