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 test for precompiled binary failing on Windows to CI #389

Merged
merged 8 commits into from
Feb 25, 2023
Merged
Changes from 2 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
70 changes: 69 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ jobs:
- run-on-clap
- run-on-sqllogictest
- run-on-ref-slice-fork
- run-on-ref-slice-fork-windows
steps:
- run: exit 0
obi1kenobi marked this conversation as resolved.
Show resolved Hide resolved

Expand Down Expand Up @@ -898,14 +899,81 @@ jobs:
cd semver
rm output
rm -f unexpectedly_did_not_fail

mgr0dzicki marked this conversation as resolved.
Show resolved Hide resolved
- name: Save rustdoc
uses: actions/cache/save@v3
if: steps.cache.outputs.cache-hit != 'true'
with:
key: ${{ runner.os }}-${{ hashFiles('.rustc-version', 'semver/**/Cargo.lock') }}-${{ github.job }}-rustdoc
path: subject/target/semver-checks/cache

run-on-ref-slice-fork-windows:
# Same as above, but run on a Windows machine and from a compiled binary after
# removing .cargo/registry/index. This broke in a workflow of the GitHub Action
# using a precompiled binary after a fresh Rust installation, when the
# registry index does not exist yet.
name: Run cargo-semver-checks on ref-slice fork (Windows)
runs-on: windows-latest
steps:
- name: Checkout cargo-semver-checks
uses: actions/checkout@v3
with:
persist-credentials: true

- name: Checkout ref-slice fork semver break
uses: actions/checkout@v3
with:
persist-credentials: false
repository: 'mgr0dzicki/cargo-semver-action-ref-slice'
ref: 'major_change'
path: 'subject'

- name: Install rust
uses: actions-rs/toolchain@v1
with:
toolchain: stable
profile: minimal
override: true

- uses: Swatinem/rust-cache@v2
with:
key: ref-slice-fork-windows

- name: Build cargo-semver-checks
run: cargo build

- name: Remove the cargo registry index
run: Remove-Item -Recurse -Force C:\Users\runneradmin\.cargo\registry\index
obi1kenobi marked this conversation as resolved.
Show resolved Hide resolved
obi1kenobi marked this conversation as resolved.
Show resolved Hide resolved

- name: Run cargo-semver-checks
continue-on-error: true
id: semver_checks
run: ./target/debug/cargo-semver-checks.exe semver-checks check-release --manifest-path="subject/Cargo.toml" 2>&1 | tee output

- name: Check whether it failed
if: steps.semver_checks.outcome != 'failure'
run: |
echo "Error! check-release should have failed because of the breaking change, but it has not."
exit 1

- name: Baseline is correct
run: |
$EXPECTED = " Parsing ref_slice v1.2.2 (current)`r`n Parsing ref_slice v1.2.1 (baseline)`r`n Checking ref_slice v1.2.1 -> v1.2.2 (patch change)"
# Strip ANSI escapes for colors and bold text before comparing.
$RESULT = (cat output | grep 'ref_slice v1.' | sed "s,\x1B\[[0-9;]*[a-zA-Z],,g") | Out-String
compare $RESULT $EXPECTED

- name: Semver break found
run: |
$EXPECTED = "--- failure function_missing: pub fn removed or renamed ---"
$RESULT = (cat output | grep failure)
compare $RESULT $EXPECTED
# Ensure the following fragment (not full line!) is in the output file:
grep ' function ref_slice::ref_slice, previously in file' output
Copy link
Owner

Choose a reason for hiding this comment

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

I added caching for baseline rustdoc JSON files in the other jobs -- would you mind adding it here too? You'll need to amend the top EXPECTED variable since right now it effectively asserts that caching isn't used (Parsing ref_slice v1.2.1 (baseline), not (baseline, cached)), and add the new caching steps as in the other jobs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added caching, but also extracted building the Windows binary to a separate job as you did with the one for Ubuntu. I think it makes it a little cleaner and we don't have to manually remove the index directory if we restore the cached binary instead of building it. What is your opinion?

One may ask why I created a separate build-binary-windows job instead of using matrix strategy for the existing one. The main reason is that building the tool on Windows appears to be much slower, and if we use matrix strategy, there is currently no way for other jobs to depend only on the ubuntu case, which makes the whole CI last longer...

Copy link
Owner

Choose a reason for hiding this comment

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

The explanation here is great, but once this PR merges, we probably won't remember why the jobs are separate. We might even merge them together into a matrix, even though you clearly point out how that isn't the right thing to do.

Consider adding this as a comment in the CI file, where it will be harder to miss?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea! Added in #407


- name: Cleanup
run: rm output

init-release:
name: Run the release workflow
needs:
Expand Down