Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maxim Kokryashkin <max.kokryashkin@gmail.com>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit] ci: extend tarantool integration testing
Date: Wed, 21 Feb 2024 18:21:19 +0300	[thread overview]
Message-ID: <ZdYU70W0lm-bNmh8@root> (raw)
In-Reply-To: <20240216222420.179993-1-m.kokryashkin@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

  reply	other threads:[~2024-02-21 15:25 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-16 22:24 Maxim Kokryashkin via Tarantool-patches
2024-02-21 15:21 ` Sergey Kaplun via Tarantool-patches [this message]
2024-03-01  8:25 Maxim Kokryashkin via Tarantool-patches
2024-03-04 14:05 ` Sergey Bronnikov via Tarantool-patches
2024-03-06 20:40   ` Maxim Kokryashkin via Tarantool-patches
2024-03-04 16:45 ` Sergey Kaplun via Tarantool-patches
2024-03-06 20:34   ` Maxim Kokryashkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZdYU70W0lm-bNmh8@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit] ci: extend tarantool integration testing' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox