Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP][CI] Test artifacts on different platforms #3602

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

0yi0
Copy link
Collaborator

@0yi0 0yi0 commented Jul 26, 2024

No description provided.

Copy link
Member

juntao commented Jul 26, 2024

Hello, I am a code review agent on flows.network. Here are my reviews of changed source code files in this PR.


.github/workflows/build-extensions-on-manylinux.yml

Potential issues

  1. Bug: The "Build WasmEdge" step is missing a condition to check if the cache was restored successfully before building. If the cache restoration fails, it will attempt to build without the necessary files and dependencies, which can lead to errors or incorrect builds.

  2. Bug: In the "Test ${{ matrix.plugin }}" step, there is no error handling for test failures. If a test fails, the workflow will continue to the next steps, potentially leading to false positives in the build process and not alerting about the failed test.

  3. Bug: The "Create test files" step is missing a condition to check if tests are enabled before creating the test files. If tests are disabled, this step will still attempt to create test files, which can lead to errors or unnecessary artifacts being created and uploaded.

  4. Bug: In the "Upload WasmEdge ${{ matrix.plugin }} plugin tar.gz package" step, there is no error handling for failed uploads. If the upload fails for any reason (e.g., network issues, invalid token), the workflow will continue to the next steps without alerting about the failure.

  5. Bug: The "Ensure git safe directory" step is being run multiple times in different jobs and steps, which can lead to unnecessary operations and potential conflicts. This step should be run once at the beginning of each job that requires it.

Summary of changes

  • Added two new input parameters: legacy and upload_test. Both are optional boolean values, with default values set to false.
  • Changed the value of build_tests environment variable based on the new release and upload_test inputs.
  • Modified the build step to build WasmEdge instead of any specific plugin when legacy is not enabled.
  • Introduced a new build target for packaging WasmEdge, which is used only if upload_test is set to true.
  • Added steps to create and upload test files if upload_test is set to true.
  • Renamed the step "Install dependencies" to "Prepare wasm_bpf environment", as it now prepares the environment for the 'wasm_bpf' plugin specifically.
  • Modified the artifact upload step to include the test files and WasmEdge package if upload_test is set to true.
  • Added a conditional step to install gh, which is only executed when release is set to true.

.github/workflows/reusable-build-extensions.yml

Potential issues

  1. Bug: The test_extensions_from_manylinux job is not configured to handle multiple architectures (ARM64) like the build_manylinux job, which could lead to test failures on ARM64 platforms.

This bug might cause the tests for extensions built on ARM64 platform to fail because there's no corresponding testing environment setup in test_extensions_from_manylinux.

Summary of changes

  • The build_extensions_on_manylinux job has been renamed to build_manylinux.
    • Two additional configurations have been added for ManyLinux 2.28, targeting x86_64 and aarch64 architectures.
    • A new job called test_extensions_from_manylinux has been added that depends on the build_manylinux job. This job runs tests using extensions built with ManyLinux 2.28 on an Ubuntu build environment with GCC.
    • The build_extensions_on_manylinux job now uses a different reusable workflow file path.
    • Input parameters for the version and release have been added to all jobs that use them.
    • A new input parameter called upload_test has been added to the build_manylinux job and is set to true.
    • The secrets are now inherited by all jobs that require them.
    • A new job called build_manylinux_legacy has been added, which builds extensions on legacy ManyLinux 2014 environments for x86_64 and aarch64 architectures. This job uses the same reusable workflow file as build_manylinux, but with an additional input parameter called legacy set to true.

.github/workflows/test-extensions-from-manylinux.yml

Potential issues

Based on the provided code, I've identified two potential bugs:

  1. Bug 我用docker的方式安装ssvm,但是没有找到详细的操作步骤 #1: The script in the github-script action might not correctly access the JSON object due to incorrect string interpolation syntax. It should be ${{ json_o[inputs.asset_tag] }} instead of ${json_o.list.${{ inputs.asset_tag }}}.

  2. Bug Update README.md #2: The run command in the "Test ${{ matrix.plugin }}" step is using undefined variables ${target} and ${test_dir}, which are not defined anywhere in the provided code. This could lead to runtime errors when trying to execute those commands.

Summary of changes

  • The patch adds a new reusable workflow named "Test extensions from manylinux".
  • It is triggered by the workflow_call event and accepts four inputs: runner, docker_tag, asset_tag, and version.
  • Two jobs are defined: prepare and test.
  • The prepare job reads a JSON file named "matrix-extensions.json" in the ".github/workflows/" directory, parses it to find the list of plugins for the given asset_tag, and sets this as an output variable that can be used by other jobs.
  • The test job uses the matrix strategy to run tests for each plugin found in the previous step. It runs on the provided runner and container (with privileged mode enabled for wasm_bpf plugin).
  • For each plugin, it downloads an artifact, extracts it, moves the target file into a test directory, and then runs the tests by executing the ${{ matrix.plugin }}Tests command.
  • The environment variable test_dir is set to "test/plugins/${{ matrix.plugin }}" for each test run.

@github-actions github-actions bot added the c-CI An issue related to CI label Jul 26, 2024
@0yi0 0yi0 force-pushed the yi/test-assets branch 6 times, most recently from 37190ca to b43941c Compare August 1, 2024 15:40
Signed-off-by: Yi Huang <yi@secondstate.io>
@0yi0 0yi0 force-pushed the yi/test-assets branch 3 times, most recently from ea362b7 to 82fb464 Compare August 2, 2024 11:06
Signed-off-by: Yi Huang <yi@secondstate.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-CI An issue related to CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants