Skip to content

Fix and tweak integration tests #2715

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 20, 2018
Merged

Fix and tweak integration tests #2715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 12 additions & 15 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,34 +19,31 @@ matrix:
- env: CFG_RELEASE_CHANNEL=beta
- os: osx
- env: INTEGRATION=cargo
- env: INTEGRATION=chalk
- env: INTEGRATION=rust-clippy
- env: INTEGRATION=mdbook
- env: INTEGRATION=stdsimd
- env: INTEGRATION=rust-semverver
- env: INTEGRATION=chalk
- env: INTEGRATION=crater
- env: INTEGRATION=futures-rs
- env: INTEGRATION=rand
- env: INTEGRATION=failure
- env: INTEGRATION=error-chain
- env: INTEGRATION=bitflags
- env: INTEGRATION=log
- env: INTEGRATION=glob
- env: INTEGRATION=tempdir
- env: INTEGRATION=rust-semverver
allow_failures:
- env: INTEGRATION=cargo
- env: INTEGRATION=stdsimd
- env: INTEGRATION=mdbook
# PR sent
- env: INTEGRATION=crater
- env: INTEGRATION=rust-semverver
- env: INTEGRATION=rust-clippy
- env: INTEGRATION=chalk
- env: INTEGRATION=bitflags
- env: INTEGRATION=error-chain
- env: INTEGRATION=failure
- env: INTEGRATION=futures-rs
- env: INTEGRATION=log
# #2721
- env: INTEGRATION=rand
- env: INTEGRATION=glob
- env: INTEGRATION=tempdir
# dues to a test failure (fails before Rustfmt'ing too)
- env: INTEGRATION=stdsimd
# Need to run an lalrpop build step before testing?
- env: INTEGRATION=chalk
# Doesn't build
- env: INTEGRATION=rust-semverver

before_script:
- |
Expand Down
24 changes: 7 additions & 17 deletions ci/integration.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@ set -ex

: ${INTEGRATION?"The INTEGRATION environment variable must be set."}

# FIXME: this is causing the build to fail when rustfmt is found in .cargo/bin
# but cargo-fmt is not found.
# FIXME: this means we can get a stale cargo-fmt from a previous run.
#
# `which rustfmt` fails if rustfmt is not found. Since we don't install
# `rustfmt` via `rustup`, this is the case unless we manually install it. Once
Expand All @@ -15,12 +14,14 @@ set -ex
# here after the first installation will find `rustfmt` and won't need to build
# it again.
#
# which rustfmt || cargo install --force
#which cargo-fmt || cargo install --force
cargo install --force

echo "Integration tests for: ${INTEGRATION}"
cargo fmt -- --version

function check_fmt {
touch rustfmt.toml
cargo fmt --all -v 2>&1 | tee rustfmt_output
if [[ $? != 0 ]]; then
cat rustfmt_output
Expand All @@ -45,35 +46,24 @@ function check_fmt {
fi
}

function check {
cargo test --all
if [[ $? != 0 ]]; then
return 1
fi
check_fmt
if [[ $? != 0 ]]; then
return 1
fi
Copy link
Contributor

@gnzlbg gnzlbg May 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Note that stdsimd was broken because the doc tests failed to build after a reformat. IMO we should be checking that if the library builds correctly before formatting, it should also build correctly afterwards, and that applies to all its tests, including doc tests.

Running the tests is not necessary, but the time it takes to run the tests of these libraries is negligible compared to the time it takes to build the tests. And knowing whether formatting changes alter the results of the tests in these libraries allows us to at least ping their maintainers on the nursery, or send PRs when things break.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. I'll add that back in once I've worked out what is going on with the rustfmt failures

Copy link
Contributor

Choose a reason for hiding this comment

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

Many of the failures are either warnings because the crates need pull request to remove deprecated options, or because rustfmt emits warnings or errors that the user cannot do anything about.

If there is nothing that the user can do about a warning or internal error, then the diagnostic should IMO be opt-in (e.g. behind a rustfmt --debug-output or similar).

I've tried to enable these diagnostics here because even if they are opt-in, they should be fixed at some point. However a "red" allowed to fail travis build bot because of a warning can hide a change that actually breaks the build so... I did not know what the right way to proceed was here and did not wanted to add all buildbots twice...

Copy link
Member Author

Choose a reason for hiding this comment

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

What's puzzling me is that some of the errors are meant to be opt-in with --error-on-unformatted and when I run locally I don't see the errors, but running on CI is doing something different to my local setup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe is a different rustfmt being picked up? I had that problem when I enabled the cargo cache on travis, so I disabled it. Or maybe your local rustfmt is different than the one here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured it out! The directories here are nested under rustfmt, whereas my local tests were not, that means they see rustfmt's rustfmt.toml which opts into the debug mode stuff. Testing now without that...

}

case ${INTEGRATION} in
cargo)
git clone --depth=1 https://github.com/rust-lang/${INTEGRATION}.git
cd ${INTEGRATION}
export CFG_DISABLE_CROSS_TESTS=1
check
check_fmt
cd -
;;
failure)
git clone --depth=1 https://github.com/rust-lang-nursery/${INTEGRATION}.git
cd ${INTEGRATION}/failure-1.X
check
check_fmt
cd -
;;
*)
git clone --depth=1 https://github.com/rust-lang-nursery/${INTEGRATION}.git
cd ${INTEGRATION}
check
check_fmt
cd -
;;
esac