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

Add unit test ci job #3 #12

Merged
merged 3 commits into from
Feb 5, 2024
Merged

Add unit test ci job #3 #12

merged 3 commits into from
Feb 5, 2024

Conversation

rjtch
Copy link
Contributor

@rjtch rjtch commented Jan 31, 2024

added pipeline for tests

@davidB
Copy link
Collaborator

davidB commented Feb 1, 2024

@rjtch I prefer to have as few as possible workflow files (eg 1 ci.yml file) with all the job/steps to the ci, especially if they

  • used the same triggers
  • used the same setup (aka the setup of the developer, who should be able to run the same on local)
  • are not shared/reused by other repo,...

I also try to limit as much as possible setup that could not be reproduced by the developer locally (=> why I prefer mise or devcontainer over using a tool that configures toolchain on for ci like setup-rust-toolchain, install-action, including cargo-make(made by me :-D)). FYI, mise could be installed in ci with curl (in this case, there is no cache).

Creating concurrent jobs (with matrix) as done for lint and tests (in this PR) looks friendly in terms of visual report and execution time (in parallel). But I prefer sequential with fewer jobs because:

  • fewer resource usage (fewer machines to up, fewer times to run the setup, less download/upload of cache, and less conflict on shared cache), we can stop earlier (on the first failure.
  • avoid to re-run several time some transitive action (build dependencies, generate code, build,...)
  • more straightforward to "matrix" for multiple environments (windows, linux-x86_64, linux-arm64,...)

I'm sure in conventional commit we should use prefix ci: (or build) instead of feat(ci):.

@davidB
Copy link
Collaborator

davidB commented Feb 1, 2024

jobs:
  tests:
    runs-on: ${{ matrix.os }}
    strategy:
      fail-fast: false
      matrix:
        os: [ubuntu-latest]
    env:
      CARGO_TERM_COLOR: always
      RUST_BACKTRACE: full
    steps:
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0
          submodules: "true"
      # - uses: jdx/mise-action@v2
      - name: install mise until mise-action is allowed
        run: |
          curl https://mise.jdx.dev/install.sh | sh
          mise install
      - name: Rust Cache
        uses: actions/cache@v4
        continue-on-error: false
        with:
          path: |
            ~/.cargo/bin
            ~/.cargo/registry
            ~/.cargo/git/db/
          key: ${{ runner.os }}-sdk-rust-${{ hashFiles('**/Cargo.toml') }} # no Cargo.lock in this repo
          restore-keys: |
            ${{ runner.os }}-sdk-rust-
      - run: make generate
      - run: make check_no_uncommitted_changes_on_sdk
      - run: make test
      - run: make check

@davidB
Copy link
Collaborator

davidB commented Feb 1, 2024

reminder: Cargo.lock are not committed for lib (only for application). So don't expect to use it in any action, command launched by ci (cache, call to make,...)

@rjtch rjtch force-pushed the Add_unit_test_CI_job_#3 branch from 8bfd992 to ab3f106 Compare February 2, 2024 19:20
@rjtch rjtch requested a review from afrittoli February 5, 2024 14:32
Comment on lines +26 to +29
- name: install mise until mise-action is allowed
uses: taiki-e/install-action@v2
with:
tool: mise, cargo-nextest, cargo-hack
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let me know which action you need exactly and I will add it to the configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The action is already added taiki-e/install-action@v2 you has added it in my last pr. This action helps downloading tools necessary for the flow. They are used in the Makefile.

@afrittoli
Copy link
Contributor

Thanks for all the updates!

@afrittoli afrittoli merged commit 06d5076 into main Feb 5, 2024
7 checks passed
@davidB davidB deleted the Add_unit_test_CI_job_#3 branch March 7, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants