Skip to content
This repository has been archived by the owner on Nov 12, 2022. It is now read-only.

Infer integration tests under tests/ #386

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Xanewok
Copy link
Contributor

@Xanewok Xanewok commented Nov 28, 2017

By default Cargo treats tests/*.rs as integration tests, so there's no need to explicitly specify them in Cargo.toml.


This change is Reviewable

@Xanewok
Copy link
Contributor Author

Xanewok commented Nov 28, 2017

Tests are indeed detected, but typedarray randomly failed under nightly-x86_64-pc-windows-msvc:

     Running target\debug\deps\typedarray-47ec600ef76b3e7a.exe
running 2 tests
test typedarray ... FAILED
test typedarray_update_panic ... ok
failures:
---- typedarray stdout ----
	thread 'typedarray' panicked at 'called `Result::unwrap()` on an `Err` value: ()', src\libcore\result.rs:906:4
failures:
    typedarray
test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '--test typedarray'

https://ci.appveyor.com/project/Ms2ger/rust-mozjs/build/1.0.485/job/vytvpct6tyxx96xa#L242

@jdm
Copy link
Member

jdm commented Nov 28, 2017

My suspicion is that when the tests are not declared in this way, they are run using multiple threads.

@fitzgen
Copy link
Contributor

fitzgen commented Nov 28, 2017

Could wrap each test in a thread::spawn(|| { ... }).

@jdm
Copy link
Member

jdm commented Nov 28, 2017

How would that help?

@fitzgen
Copy link
Contributor

fitzgen commented Nov 28, 2017

So that we know that we only ever create a single JSRuntime on any given thread. If teh default is spreading #[test]s across some thread pool, it could end up creating multiple JSRuntimes on the same physical thread which is not permitted.

@Xanewok
Copy link
Contributor Author

Xanewok commented Nov 28, 2017

@jdm explicitly stating test targets in Cargo.toml doesn't change the way they are built. The way Cargo works here is that it builds each integration test as a separate binary and runs #[test]s inside in parallel, across multiple threads (unless overriden). This means that tests like typedarray.rs containing both tests do execute in the same process in multiple threads here, regardless if the targets are specified in Cargo.toml file.

@Xanewok
Copy link
Contributor Author

Xanewok commented Nov 28, 2017

Since this tests bindings to an external C library that has to guarantee that single JSRuntime per physical thread, i think this is one of the rare cases were testing with explicit RUST_TEST_THREADS=1 (seems to be replaced with cargo test -- --test-threads=1 nowadays?) makes sense.

Seems that executing with --test-threads=1 is within margin of error on my laptop and test suite execution is ~0.5s with and without this option.

@fitzgen
Copy link
Contributor

fitzgen commented Nov 28, 2017

Have we verified that forcing one JSRuntime per thread actually does fix the aforementioned issues? Otherwise we may be getting ahead of ourselves :)

@Xanewok
Copy link
Contributor Author

Xanewok commented Nov 29, 2017

This is intermittent and I couldn't reproduce it locally myself. I might leave it to run longer and see if the parallel version doesn't encounter failures, but I'm not really experienced with SM and JSAPI, so I'm not really sure what is the expected environment in which SM is guaranteed to run 😢

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #382) made this pull request unmergeable. Please resolve the merge conflicts.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #430) made this pull request unmergeable. Please resolve the merge conflicts.

@meteor-lsw
Copy link
Contributor

@Xanewok @jdm

When I executed the 'cargo test' command, I got an unexpected result. The tests in rooting.rs were skipped. Can this PR help me solve this problem?

     Running target/debug/deps/rooting-2ed6793bd8bb11f6

running 0 tests

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

@jdm
Copy link
Member

jdm commented Dec 22, 2018

No idea. I've never seen that before.

@meteor-lsw
Copy link
Contributor

Maybe this is a bug of cargo.

@Xanewok
Copy link
Contributor Author

Xanewok commented Dec 22, 2018

Doesn't

#![cfg(feature = "debugmozjs")]

effectively prune any code in this module unless you test with debugmozjs feature? I assume Cargo still collects that file and compiles it but there may be nothing for rustc's libtest to test/run anymore at that point.

@jdm
Copy link
Member

jdm commented Dec 22, 2018

Ah, yes. That is correct.

@meteor-lsw
Copy link
Contributor

Got it, thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants