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 comments below.

On 01.03.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.
> 
> As noted in the documentation[1], it is impossible for a single

Typo: s/[1]/ [1]/

> workflow call tree to include more than 20 workflows. To
> overcome this limitation, workflows are started in a bunch of
> separate files. This makes it impossible to depend on LuaJIT-only
> jobs for integration workflow since it is impossible to make
> cross-dependencies between workflow files.
> 
> The name of a callee workflow cannot be substituted using a
> matrix[2], so workflow calls are copied and pasted instead.

Please add the corresponding comment to the workflow file too.

Typo: s/[2]/ [2]/

> 
> Table below shows which workflows are enabled and why.
> 
> Workflow name                   |+/-| Reason
> -----------------------------------------------------------------

<snipped>

> default_gcc_centos_7            | + | Ancient gcc build.

Agree with Sergey here -- "gcc version (7)" looks better.

<snipped>

> luajit-integration              | + | Exotic LuaJIT options.
> memtx_allocator_based_on_malloc | + | Not relevant to LuaJIT.

Typo: s/+/-/

> osx                             | - | Nightly build.
> out_of_source                   | - | Not relevent to LuaJIT.

I don't agree here. We want to check that our tests work in OOS builds
too. It may be an issue for some libraries to be built or for some
tests that use generated files, etc.

If we enable this, we can skip static_build_cmake_linux workflow, as
Sergey suggested (since it is the same as static_build, except it is OOS
build). But I'm not against too leave that workflow as well.

Also, looks like it is used, see below [*].

<snipped>

> static_build                    | + | Tarantool static build.
> static_build_cmake_linux        | + | Tarantool static build.

<snipped>

> [1]: https://docs.github.com/en/actions/using-workflows/reusing-workflows#limitations
> [2]: https://github.com/orgs/community/discussions/45342#discussioncomment-4779360
> ---
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/integration-testing
> 
>  .github/workflows/exotic-tarantool.yml    |  62 ++++++++++++
>  .github/workflows/tarantool-ecosystem.yml |  35 +++++++
>  .github/workflows/testing.yml             | 116 ++++++++++++++++------
>  3 files changed, 183 insertions(+), 30 deletions(-)
>  create mode 100644 .github/workflows/exotic-tarantool.yml
>  create mode 100644 .github/workflows/tarantool-ecosystem.yml
> 
> diff --git a/.github/workflows/exotic-tarantool.yml b/.github/workflows/exotic-tarantool.yml
> new file mode 100644
> index 00000000..be55291a
> --- /dev/null
> +++ b/.github/workflows/exotic-tarantool.yml

Minor: I suggest using tarantool-exoting to be consistent with
tarantool-ecosystem. Within this rename, don't forget to rename the job
for consistency.

OTOH, maybe it is better to name this workflow like the
tarantool-integration or luajit-integration to show that we run
**LuaJIT** tests under Tarantool. What name do you like the most?

<snipped>

> +jobs:
> +  test-exotic-tarantool:

Why do you separate this job from the testing.yml workflow? Also, it
should depend on luajit-testing, so it still should be placed in the
<testing.yml> shouldn't it? If not, please add a rationale in the
comment.

> +    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 }}
> +    # XXX: Only LuaJIT-tests are running in this workflow.

Side note: What do you think about adding the FreeBSD runs to that
workflow?
It doesn't cost much since LuaJIT-tests are tiny. And we already run
such tests for OSX.

> +    uses: tarantool/tarantool/.github/workflows/luajit-integration.yml@master
> +    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-ecosystem.yml b/.github/workflows/tarantool-ecosystem.yml
> new file mode 100644
> index 00000000..82a47fe1
> --- /dev/null
> +++ b/.github/workflows/tarantool-ecosystem.yml

<snipped>

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

Do I understand correctly that this workflow is separated from testing
integration list since it has reached the limit mentioned above (20
workflows)? If so, please mention this here as a comment and in the
testing.yml too.

> +

Nit: Excess empty line.

> diff --git a/.github/workflows/testing.yml b/.github/workflows/testing.yml
> index cb4ba57b..7bd527be 100644
> --- a/.github/workflows/testing.yml
> +++ b/.github/workflows/testing.yml
> @@ -83,37 +83,93 @@ jobs:
>          run: cmake --build . --parallel --target LuaJIT-test
>          working-directory: ${{ env.BUILDDIR }}
> 
> +  test-tarantool-coverage:

Maybe it is better to add names for better readablility.
Something like Tarantool intergration coverage, here and below.

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

<snipped>

> +  test-tarantool-debug:
> +    needs: test-luajit
> +    uses: tarantool/tarantool/.github/workflows/debug.yml@master
> +    with:
> +      submodule: luajit
> +      revision: ${{ github.sha }}
> +
> +  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
> +    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 }}

[*] This workflow is mentioned as not used.

> +
> +  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 }}
> --
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun