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

Improve CI workflows #28

Merged
merged 4 commits into from
Dec 9, 2024
Merged

Improve CI workflows #28

merged 4 commits into from
Dec 9, 2024

Conversation

User344
Copy link
Member

@User344 User344 commented Dec 4, 2024

In this PR I changed event type due to number of reasons:

  • Main reason is to allow CI workflow to run for PRs that are based on branches in forks.
  • Also fixes issue where CI wouldn't have ran if merging is not possible due to conflicts (we didn't run into this though).

And I also bumped ubuntu runner version to 24.04 (latest) to get rid of a warning in logs.
image

@User344 User344 requested a review from artob as a code owner December 4, 2024 20:03
@User344 User344 added bug Something isn't working enhancement New feature or request labels Dec 4, 2024
@User344
Copy link
Member Author

User344 commented Dec 5, 2024

Added installing of protoc 29.1 to fix issue in #27
image

@artob artob self-assigned this Dec 5, 2024
Copy link
Contributor

@SamuelSarle SamuelSarle left a comment

Choose a reason for hiding this comment

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

LGTM.

Sidenote while we're looking at this file:
Is this correct?:

      - name: Run tests
        id: run-tests
        if: ${{ steps.check-tests.outcome == 'success' }}
        run: |
          cargo +${{ steps.install-rust.outputs.name }} test --target ${{ matrix.target }} --workspace --test "*" --no-fail-fast

It seems like providing --test "*" means that it only runs tests found in **/tests/*.rs but doesn't test anything in src/ folders. See the difference in output of cargo test --test "*" and cargo test in repo root folder.
Also if you give it a bogus name it lists only the four tests in tests/*.rs files:

$ cargo test --test "foobar"

error: no test target named `foobar`.
Available test targets:
    mpsc
    zst
    json_roundtrip
    function_block

Copy link
Member

@artob artob left a comment

Choose a reason for hiding this comment

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

LGTM

@artob artob changed the title Minor CI fixes Improve CI workflows Dec 9, 2024
@artob artob merged commit cb505e5 into asimov-platform:master Dec 9, 2024
@User344
Copy link
Member Author

User344 commented Dec 10, 2024

LGTM.

Sidenote while we're looking at this file: Is this correct?:

      - name: Run tests
        id: run-tests
        if: ${{ steps.check-tests.outcome == 'success' }}
        run: |
          cargo +${{ steps.install-rust.outputs.name }} test --target ${{ matrix.target }} --workspace --test "*" --no-fail-fast

It seems like providing --test "*" means that it only runs tests found in **/tests/*.rs but doesn't test anything in src/ folders. See the difference in output of cargo test --test "*" and cargo test in repo root folder. Also if you give it a bogus name it lists only the four tests in tests/*.rs files:

$ cargo test --test "foobar"

error: no test target named `foobar`.
Available test targets:
    mpsc
    zst
    json_roundtrip
    function_block

Good catch! It is indeed a little bit wrong!
For context:
Running cargo test will not only run tests, but also doctests (obviously) but also it will compile(not run) examples for some reason. Meaning that if there are issues with examples, both "Run tests" and "Build examples" steps will fail, though there is nothing wrong with tests!
That's exactly why --test "*" argument was introduced. Quotes are required so shell doesn't treat it as glob. And when Cargo encounters it, it will only run the tests, and tests only. Now I am not quite sure why I decided to test tests only without doctests back then...
Either way, good job on noticing and testing this! I will start working on a fix now.

@User344 User344 mentioned this pull request Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants