Skip to content

cargo test fails from crates.io tarball #38

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

Jump to bottom
Closed
ignatenkobrain opened this issue Jun 20, 2019 · 28 comments · Fixed by #48
Closed

cargo test fails from crates.io tarball #38

ignatenkobrain opened this issue Jun 20, 2019 · 28 comments · Fixed by #48

Comments

@ignatenkobrain
Copy link 

+ /usr/bin/env CARGO_HOME=.cargo RUSTC_BOOTSTRAP=1 /usr/bin/cargo test -j48 -Z avoid-dev-deps --release --no-fail-fast
BUILDSTDERR:        Fresh smallvec v0.6.10
BUILDSTDERR:    Compiling unicode-normalization v0.1.8 (/builddir/build/BUILD/unicode-normalization-0.1.8)
BUILDSTDERR:      Running `/usr/bin/rustc --crate-name unicode_normalization src/lib.rs --color never --emit=dep-info,link -C opt-level=3 --test -C metadata=43229941e7e0e6a1 -C extra-filename=-43229941e7e0e6a1 --out-dir /builddir/build/BUILD/unicode-normalization-0.1.8/target/release/deps -L dependency=/builddir/build/BUILD/unicode-normalization-0.1.8/target/release/deps --extern smallvec=/builddir/build/BUILD/unicode-normalization-0.1.8/target/release/deps/libsmallvec-8bcaab121ba3708c.rlib -Copt-level=3 -Cdebuginfo=2 -Clink-arg=-Wl,-z,relro,-z,now -Ccodegen-units=1`
BUILDSTDERR: error[E0583]: file not found for module `test`
BUILDSTDERR:   --> src/lib.rs:75:5
BUILDSTDERR:    |
BUILDSTDERR: 75 | mod test;
BUILDSTDERR:    |     ^^^^
BUILDSTDERR:    |
BUILDSTDERR:    = help: name the file either test.rs or test/mod.rs inside the directory "src"
BUILDSTDERR: error: aborting due to previous error
BUILDSTDERR: For more information about this error, try `rustc --explain E0583`.
BUILDSTDERR: error: Could not compile `unicode-normalization`.
BUILDSTDERR: Caused by:
BUILDSTDERR:   process didn't exit successfully: `/usr/bin/rustc --crate-name unicode_normalization src/lib.rs --color never --emit=dep-info,link -C opt-level=3 --test -C metadata=43229941e7e0e6a1 -C extra-filename=-43229941e7e0e6a1 --out-dir /builddir/build/BUILD/unicode-normalization-0.1.8/target/release/deps -L dependency=/builddir/build/BUILD/unicode-normalization-0.1.8/target/release/deps --extern smallvec=/builddir/build/BUILD/unicode-normalization-0.1.8/target/release/deps/libsmallvec-8bcaab121ba3708c.rlib -Copt-level=3 -Cdebuginfo=2 -Clink-arg=-Wl,-z,relro,-z,now -Ccodegen-units=1` (exit code: 1)

I would appreciate if you would just make it under some cfg or something like that so that it does not fail for us.
@kentfredric
Copy link
Contributor

I guess this is slightly ironic.

The Cargo.toml explicitly strips some tests, and its the stripping of these tests that causes cargo test to fail.

Please endeavour to have some sort of useful tests in the shipped crate. Its important for linux-vendor QA purposes ( pretend linux vendors are a kind of upstream who have to make sure everything works on a new rust the same way you would, but have to use a shipped crate as a baseline )

@Manishearth
Copy link
Member

Adding tests to this crate bloats the size of the published crate (which matters for many consumers), they were explicitly stripped because the tests rely on unicode tables.

@kentfredric
Copy link
Contributor

That's fine, just I find it funny, as the problem emerges from there being an incomplete exorcising of tests :D

Though it would be nice if there was some medium ground, where there's enough test in place to provide something useful to end users, and having more exhaustive tests gated behind something so as not to cause that kind of pain.

@nox
Copy link

nox commented Oct 28, 2019

Both is_combining_mark and UnicodeNormalization are public items of this crate, and they are the only two things used by the test module.

As far as I can tell, this module and the stuff from normalization_tests can be moved to an integration test in tests/ to be properly excluded from the published package, letting downstream vendors run the other tests successfully.

@Manishearth
Copy link
Member

Manishearth commented Oct 28, 2019 via email

@kentfredric
Copy link
Contributor

Yeah, turns out to be a little more complicated than nox thought.

I've managed so far to move a bunch of the tests to tests/, in a way that would make it more easy to nuke the ones that were "heavy" without consequence.

Just a few stragglers remain that annoyingly require access to private interfaces to function.

Got a few potential ways of making this work though, one idea is a build.rs that probes for the existence of the normalization_tests.rs file, and passes a flag to enable the relevant tests.

That aught to work transparently for both authors and consumers.

@Manishearth
Copy link
Member

Manishearth commented Oct 28, 2019 via email

@kentfredric
Copy link
Contributor

well... you don't have to use build.rs, just means you'll need RUST_FLAGS="--cfg bloaty_test" cargo test to make those tests spark up ;)

@kentfredric kentfredric mentioned this issue Oct 28, 2019
Closed
@Manishearth
Copy link
Member

Given that these tests really really should be run by default that solution is not acceptable either.

@Manishearth
Copy link
Member

Ultimately, "we want to run cargo test from packaging" is not a use case we particularly care about, I'm fine supporting it but it should not make things worse for the default use case. Those tests are the important ones, the other tests don't really matter much. If you're excluding those I don't even see the point of running cargo test.

It's a pretty common exercise to exclude test files from the published package, fwiw. Some of the regex dependencies do this as well since they include large text corpuses.

@kentfredric
Copy link
Contributor

not a use case we particularly care about

That translates to "Do not care about linux vendors at all", which translates to limiting your own adoption ( and seriously limits the quality of rust at large, as there's plenty of crates that do fail tests for reasons that are real problems, and we can expect to see this get worse when features get deprecated )

Some of this misery is worse than it aught to be because there's no good way to have tests outside src/ which share the visibility of src/

Abusing #[path = "../../foo.rs"] in the right places kinda works, but its also probably gonna get me yelled at.

If tests could be entirely in tests/, then you could employ the exclude rules with complete immunity, nothing bad could happen because you can safely nuke anything outside src/ without risking it breaking src/

@kentfredric
Copy link
Contributor

Something that would be gross, but survivable, is repeat the PR, but with the flag inverted. That way at least, people who want to run tests downstream only have to dance with setting RUSTFLAGS=, not with having to patch the entire crate. ( But it would be by default, still horribly broken )

@Manishearth
Copy link
Member

That translates to "Do not care about linux vendors at all", which translates to limiting your own adoption

You haven't sufficiently made the case for why you need to run tests here. You're effectively asking for cargo test for this crate to work but not run any important tests, which seems very illogical to me: why run cargo test at all? You're not getting anything out of it.

Similarly, why not run ./scripts/unicode.py followed by cargo test?

If you can figure out a way to stick the normalization tests in tests/ that would be fine.

@Manishearth
Copy link
Member

But it would be by default, still horribly broken

This comes off as super arrogant given that your original solution was to make this crate similarly horribly broken for developers. The normalization tests are the important tests, if they're not run by default there's not much point to having them. We're not going to flag off the main tests in this crate to support your use case. That's ridiculous.

@Manishearth
Copy link
Member

Here are my constraints:

  • nothing in build.rs since that affects regular users
  • running cargo test in this repo should continue to run all tests
  • ideally, all of this is rather transparent and hard to mess up

You can still use #[doc(hidden)] to move tests (and quick_check.rs) into the integration test folder if you'd like. but that's not going to give you useful tests to run from packaging. Make sure it's done in a way that keeps quick_check a dev dependency. I'm okay with adding some useful tests that run by default if someone writes them.

@kentfredric
Copy link
Contributor

You haven't sufficiently made the case for why you need to run tests here. You're effectively asking for cargo test for this crate to work but not run any important tests, which seems very illogical to me: why run cargo test at all? You're not getting anything out of it.

Not being able to run "the most important tests" is still, agreeably, not ideal.

But in general, being able to run some tests, is better than being able to run no tests.

If we're desperate and tests can't remotely be feasible to run, a last ditch alternative is "Just have something run cargo build at some point before we say its fine, at least we know it compiles".

But having some tests is better than that.

Similarly, why not run ./scripts/unicode.py followed by cargo test?

If you can figure out a way to stick the normalization tests in tests/ that would be fine.

Mostly because adding python to the build chain makes matters more complicated. Doable, maybe, but as-is, the exclusion rules remove more than just that file.

unicode-normalization/Cargo.toml

Line 21 in c469e87

exclude = [ "target/*", "Cargo.lock", "scripts/tmp", "*.txt", "src/normalization_tests.rs", "src/test.rs" ]

It removes the src/test.rs that consumes that file as well, which presently makes "just regenerate the file" something that doesn't work.

This comes off as super arrogant given that your original solution was to make this crate similarly horribly broken for developers. The normalization tests are the important tests, if they're not run by default there's not much point to having them. We're not going to flag off the main tests in this crate to support your use case. That's ridiculous.

I have recurring problems where I seem about 1000% more an ass than I am. Its @nox 's fault, I swear. Something about too many fires to put out.

But to re-iterate what that proposal does:

  • Changes nothing for general users who check out the published crate and run cargo test, it is still broken.
  • Changes nothing for maintainer side, because the flag isn't set by default by anything, and so that test still runs as default
  • Vendors who want tests to work without labourious levels of crate hacking/patching, just set RUSTFLAGS vendor side themselves, without any build.rs involvement.

I'm under no illusions this is a perfect solution, but the goal at this point is to merely improve the status-quo for vendor's from "no tests" or "compile-only testing" to "something tangentially plausible to break".

Improving that situation so that vendors and end users get useful tests would be something that could happen down the road, but I'm at a loss how to do this presently within the tools currently at our disposal.

Especially seeing the motivation for excluding this file is "keep the crate size down", and there's no real way I can see that happening while also providing comprehensive tests for vendors and other end users.

Maybe this data can be somehow stored in a packed form that can be loaded more efficiently or something, but doing that is currently beyond my skills/patience.

Make sure it's done in a way that keeps quick_check a dev dependency.

I don't see a dev-dependency presently for quick_check. Although I know there's a quickcheck crate, it seems that term is unrelated to how its used within this crate. (And I'm glad of this, that's a yuuuge crate in terms of dependencies 😉 ), but its an easy enough mistake to make, that's what I thought it was the first 10 times I saw it 😄 .

@nox
Copy link

nox commented Oct 28, 2019

I could be completely wrong but from my few interactions with vendors, it seems like it is valuable to them to have cargo test not fail for mere administrative reasons (if I may say), even if the alternative is simply for this command to not run any test at all, but I could be wrong.

On a funny side note, I too thought this crate was depending on quichcheck multiple times, hah.

@kentfredric
Copy link
Contributor

not fail for mere administrative reasons

More-or-less. The context that may help is:

  • New version of X language comes out
  • We have to have an automated run that tries to give some tangential semblance of sanity that all packages that exist for that language, and previously did not fail, still continue not failing.

But diagnosing that is much more straight forward if the "previous" state didn't fail.

And we have to run this for thousands of packages at a time.

And that's why "some tests, no matter how scant, are better than no tests, and also better than broken-by-design tests". ( not intended to be malicious, just, as is, design constraints and criteria are why tests are currently broken )

Its all about making the squeakyest wheels stand out. Just presently "test suite is not working" is highly squeaky. 😉

kentfredric added a commit to kentfredric/unicode-normalization that referenced this issue Oct 28, 2019
@kentfredric
Add "opt-out" test gating for vendors, document getting working tests
2f98e87 
Hopefully this provides the best of all worlds.

Importantly, this re-introduces src/test.rs, as that's something that
can't be trivially generated.

Both approaches are documented so the path to the easiest working
solution is clear for vendors, and they can pick either the more
expensive solution, or the simpler (but less comprehensive) one.

If this PR meets general approval, I'd like to add a .travis.yml
step that automatically tests the --cfg minimal_tests path, so at
least, that will be a "stable" target, as testing that the deployed
crate is free from errors is presently too much work to expect the
average maintainer or contributor to remember to do.

Closes: unicode-rs#38
This was referenced Oct 28, 2019
Closed
Open
@Manishearth
Copy link
Member

As mentioned in the PR, I feel like the solution I proposed with doc(hidden) will work out better for your use case, though I'm fine with using cfg instead.

@kentfredric
Copy link
Contributor

As mentioned in the PR, I feel like the solution I proposed with doc(hidden) will work out better for your use case, though I'm fine with using cfg instead.

I'm not sure I fully understand what you're suggesting here.

Are you suggesting splitting the file src/normalization_tests.rs to be an independent crate, and then simply [dev-dependencies] it into place?

Wherein you're suggesting #[doc(hidden)] so that people don't try to use that crate for things outside its intended purpose?

That could work, I guess.

@Manishearth
Copy link
Member

No, I'm proposing src/normalization_tests.rs be generated into tests/, and all of its private in-crate dependencies be reexported from a public module that is doc(hidden)

@kentfredric
Copy link
Contributor

Oh, I see, a test-proxy of sorts. That should work nicely.

@Manishearth
Copy link
Member

Yeah, something like #[doc(hidden)] pub mod __test_exports;

@kentfredric
Copy link
Contributor

This is my progress so far:

master...kentfredric:attempt-3

Any thoughts?

This currently means:

  • All end consumers have a test suite that can be expected to pass
  • All developers still have a comprehensive test suite

However, the one remaining downside is its not trivial for anyone to perform the comprehensive test suite on the published crate, as the data file, and the consumers of that data file, are all by necessity missing.

@Manishearth
Copy link
Member

@kentfredric I don't get it : why is that set of changesets moving all the tests out? Just do it for the one test causing problems. It's a much simpler change.

@Manishearth
Copy link
Member

Like, let's do this with minimal churn

@kentfredric
Copy link
Contributor

Redone:

master...kentfredric:attempt-4

This time only migrated the ones that required NORMALIZATION_TESTS

Also migrated all those tests to a single unit in order to keep the test output looking lean.

This is what the test output looks like.

    Finished test [unoptimized + debuginfo] target(s) in 1.96s
     Running target/debug/deps/unicode_normalization-10042fef5363d21b

running 11 tests
test quick_check::tests::test_stream_safe_nfc ... ok
test normalize::tests::test_hangul_composition ... ok
test quick_check::tests::test_stream_safe_nfd ... ok
test test::test_is_combining_mark_ascii ... ok
test test::test_is_combining_mark_misc ... ok
test test::test_nfd ... ok
test test::test_nfc ... ok
test test::test_nfkc ... ok
test stream_safe::tests::test_simple ... ok
test test::test_nfkd ... ok
test stream_safe::tests::test_classify_nonstarters ... ok

test result: ok. 11 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

     Running target/debug/deps/tests-09ce160129ab5819

running 3 tests
test test_normalization_tests_unaffected ... ok
test test_quick_check ... ok
test test_official ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

   Doc-tests unicode-normalization

running 1 test
test src/lib.rs -  (line 15) ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Does that look PR-ready yet or is there anything else significant I need to handle still?

@Manishearth
Copy link
Member

@kentfredric that looks great! Thanks!

@kentfredric kentfredric mentioned this issue Nov 22, 2019
Merged
@Manishearth Manishearth mentioned this issue Feb 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@kentfredric @nox @Manishearth @ignatenkobrain