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

test: fix test failures on Windows #175

Merged
merged 3 commits into from
Oct 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 97 additions & 0 deletions .github/workflows/bot.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
on:
issue_comment:
types: [created]

name: Bot

jobs:
pr_pre_comment:
# This job only runs for pull request comments
name: Comment before taking snapshots
if: ${{ github.event.issue.pull_request && github.event.comment.body == '/snapshot' }}
runs-on: ubuntu-latest
steps:
- uses: actions/github-script@v6
with:
script: |
github.rest.issues.createComment({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
body: "Start taking snapshots for this pull request.\n" +
`https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`
});
pr_snapshot:
# This job only runs for pull request comments
name: Generate a snapshot
needs: [pr_pre_comment]
if: ${{ github.event.issue.pull_request && github.event.comment.body == '/snapshot' }}
strategy:
# Do not run in parallel because we may create a new commit
max-parallel: 1
matrix:
name: [windows, linux]
include:
- name: windows
os: windows-2022
- name: linux
os: ubuntu-latest
runs-on: ${{ matrix.os }}
env:
# Set TRYCMD=overwrite to update snapshot
TRYCMD: overwrite
steps:
- uses: actions/github-script@v6
id: target-branch
with:
result-encoding: string
script: |
const pull_request = await github.rest.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number
});
return pull_request.data.head.ref;
- uses: actions/checkout@v2
with:
ref: ${{ steps.target-branch.outputs.result }}
- uses: actions-rs/toolchain@v1
with:
components: rustfmt, clippy
# Pinned to the commit hash of v2.2.1
- uses: Swatinem/rust-cache@6fd3edff6979b79f87531400ad694fb7f2c84b1f
with:
shared-key: pr-snapshot-commented-${{ matrix.name }}
- uses: actions-rs/cargo@v1
# Generate new snapshots
with:
command: test
args: cli_tests
- if: matrix.name == 'linux'
run: |
git config user.name "github-actions[bot]"
git config user.email "41898282+github-actions[bot]@users.noreply.github.com"
git commit -am "test: update snapshot for ${{ matrix.name }}" || true
git push || true
- if: matrix.name == 'windows'
run: |
git config user.name "github-actions[bot]"
git config user.email "41898282+github-actions[bot]@users.noreply.github.com"
git commit -am "test: update snapshot for ${{ matrix.name }}"
git push
pr_post_comment:
# This job only runs for pull request comments
name: Comment after taking snapshots
needs: [pr_snapshot]
if: ${{ github.event.issue.pull_request && github.event.comment.body == '/snapshot' }}
runs-on: ubuntu-latest
steps:
- uses: actions/github-script@v6
with:
script: |
github.rest.issues.createComment({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
body: 'Taking snapshots has been completed.'
});
4 changes: 2 additions & 2 deletions .github/workflows/build_binaries_on_new_releases.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ jobs:
os: macos-latest
target: x86_64-apple-darwin
- name: windows
os: windows-2019
os: windows-2022
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use windows-2022 rather than windows-latest? As I can see, .github/workflows/bot.yml use windows-latest also other platform use -latest as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for mentioning it. Because I believe that a stable binary generation is preferable, I specify windows-2022 instead of windows-latest. However, I did not notice that other platforms use the latest version. It seems that changing to latest is preferable for unification. Thus, I modified it as window-latest. I have created an issue to discuss the preferable version to support at #186.
On the other hand, I do not have a good reason for using windows-latest in bot.yml. Do you have any opinion regarding bot.yml?

Copy link
Contributor

Choose a reason for hiding this comment

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

From my opinion, we need to unify image on all of workflow. Because, the snapshot is actually not to relate building binary itself but if the image is different we are hard to identify the cause when any issue occur.
So would you mind if we use windows-2022 instead of windows-latest on other workflow for now and will discuss it on the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I think your opinion makes sense. Due to the scope of this PR, I changed os to windows-2022 for both CI and bots for the Windows platform. Let's discuss this within #186 about other platforms.

target: x86_64-pc-windows-msvc

steps:
Expand All @@ -47,7 +47,7 @@ jobs:
- name: Archive the binary for publishing on release assets
shell: bash
run: |
if [ "${{ matrix.os }}" = "windows-2019" ]; then
if [ "${{ matrix.os }}" = "windows-2022" ]; then
7z a -tzip dynein-${{ matrix.name }}.zip ./target/release/dy.exe
else
tar -C ./target/release/ -cvzf dynein-${{ matrix.name }}.tar.gz dy
Expand Down
30 changes: 29 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ name: CI

jobs:
build_and_test:
name: Rust project
name: Test and build on Linux
runs-on: ubuntu-latest
env:
RUST_LOG: debug # Output debug log
Expand Down Expand Up @@ -50,3 +50,31 @@ jobs:
with:
command: build
args: --release --all-features
build_and_test_on_windows:
name: Test and build on Windows
runs-on: windows-2022
env:
RUST_LOG: debug # Output debug log
RUST_BACKTRACE: 1 # Dump backtrace on panic
DYNEIN_TEST_NO_DOCKER_SETUP: true
# define AWS credentials in environment for test
AWS_ACCESS_KEY_ID: test
AWS_SECRET_ACCESS_KEY: test
steps:
- uses: actions/checkout@v2
- uses: actions-rs/toolchain@v1
with:
components: rustfmt, clippy
# Pinned to the commit hash of v2.2.1
- uses: Swatinem/rust-cache@6fd3edff6979b79f87531400ad694fb7f2c84b1f
with:
shared-key: build-and-test-on-windows
- uses: actions-rs/cargo@v1
with:
# Currently, we only conduct snapshot tests because GitHub Actions does not support containers in Windows.
command: test
args: cli_tests
- uses: actions-rs/cargo@v1
with:
command: build
args: --release --all-features
14 changes: 14 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1223,10 +1223,24 @@ pre-commit install
We use [trycmd](https://crates.io/crates/trycmd) to conduct snapshot testing for CLI.
If the snapshot is needed to be updated, run command;

MacOS and Linux
```shell
TRYCMD=overwrite cargo test --test cli_tests
```

Windows (PowerShell)
```powershell
$Env:TRYCMD='overwrite'
cargo test --test cli_tests
[Environment]::SetEnvironmentVariable('TRYCMD',$null)
```

Please note that we use different snapshots for the Windows environment.

### Bot
If you want to update snapshots of commands, you can use bot command `/snapshot` in your pull request.
Please note that you must type exactly as written.

## Asides

dynein is named after [a motor protein](https://en.wikipedia.org/wiki/Dynein).
Expand Down
6 changes: 5 additions & 1 deletion tests/cli_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,9 @@ use trycmd;

#[test]
fn cli_tests() {
trycmd::TestCases::new().case("tests/cmd/*.md");
if cfg!(windows) {
trycmd::TestCases::new().case("tests/cmd_windows/*.md");
} else {
trycmd::TestCases::new().case("tests/cmd/*.md");
}
}
Loading