Skip to content

Conversation

@imunproductive
Copy link
Member

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

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

imunproductive 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
Member

@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
@imunproductive
Copy link
Member Author

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.

@imunproductive imunproductive 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