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

Reexport tests without polluting namespaces #52890

Merged
merged 6 commits into from Aug 2, 2018

Conversation

Projects
None yet
4 participants
@djrenren
Copy link
Contributor

djrenren commented Jul 31, 2018

This should fix issue #52557.

Basically now we gensym a new name for the test function and reexport that.
That way the test function's reexport name can't conflict because it was impossible for the test author to write it down.
We then use a use statement to expose the original name using the original visibility.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jul 31, 2018

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jul 31, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:04:14] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:14] tidy error: /checkout/src/test/run-pass/issue-52557.rs: missing trailing newline
[00:04:16] some tidy checks failed
[00:04:16] 
[00:04:16] 
[00:04:16] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:16] 
[00:04:16] 
[00:04:16] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:16] Build completed unsuccessfully in 0:00:59
[00:04:16] Build completed unsuccessfully in 0:00:59
[00:04:16] make: *** [tidy] Error 1
[00:04:16] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0cd75afa
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:0a896663:start=1533004435080686603,finish=1533004435088991190,duration=8304587
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1a039f92
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1682e290
travis_time:start:1682e290
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0bb3e7e4
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jul 31, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:42:36] 
[00:42:36] - error: cannot test inner function
[00:42:36] -   --> $DIR/test-inner-fn.rs:15:5
[00:42:36] -    |
[00:42:36] - LL |     #[test] //~ ERROR cannot test inner function [unnameable_test_functions]
[00:42:36] -    |
[00:42:36] -    = note: requested on the command line with `-D unnameable-test-functions`
[03625020 .
2274628 ./obj
---
145460 ./obj/build/bootstrap/debug/incremental
133976 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu
133972 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release
130592 ./obj/build/bootstrap/debug/incremental/bootstrap-c7ee2tfsizs
130588 ./obj/build/bootstrap/debug/incremental/bootstrap-c7ee2tfsizs/s-f3el8jlfcp-1th506i-3t5kexjst7huj
128816 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release
122972 ./obj/build/x86_64-unknow

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jul 31, 2018

Interesting trick.

Actually, can't this work in the opposite way?

#[test]
fn foo() {}

=>

// The function itself is not changed
#[test]
fn foo() {}

// ... but the import is added.
#[some_marker_for_test_harness]
pub use foo as foo_gensym;

This would make the logic cleaner on the expander side, but looks like it would make life harder for the test harness generator since it needs to check function signatures, process should_fail attributes, etc.

if self.tests_nameable && item.attrs.iter().any(|attr| is_test_or_bench(attr)) {
let orig_vis = item.vis.clone();

// Publicize the item under gensymed name to avoid pollution

This comment has been minimized.

@petrochenkov

petrochenkov Jul 31, 2018

Contributor

Could you add a comment showing the transformation in code, like in #52890 (comment)?

item = item.map(|mut item| {
item.vis = respan(item.vis.span, ast::VisibilityKind::Public);
item.ident = Ident::from_interned_str(
item.ident.as_interned_str()).gensym();

This comment has been minimized.

@petrochenkov

petrochenkov Jul 31, 2018

Contributor
orig_ident = item.ident;
item.ident = item.ident.gensym();
let use_item = self.cx.item_use_simple_(
item.ident.span,
orig_vis,
Some(Ident::from_interned_str(item.ident.as_interned_str())),

This comment has been minimized.

@petrochenkov

petrochenkov Jul 31, 2018

Contributor

Some(orig_ident)

item.ident.span,
orig_vis,
Some(Ident::from_interned_str(item.ident.as_interned_str())),
self.cx.path(item.ident.span, vec![Ident::from_str("self"), item.ident]));

This comment has been minimized.

@petrochenkov

petrochenkov Jul 31, 2018

Contributor

keywords::SelfValue.ident()

@djrenren

This comment has been minimized.

Copy link
Contributor Author

djrenren commented Jul 31, 2018

@petrochenkov unfortunately, that change wouldn't work because we can't pub use a non-pub function. This is E0364. There is a variant of your suggestion that turns:

#[test]
fn foo(){}

into:

#[was_public|was_priv]
pub fn foo_gensym(){}

and then insert the appropriate use in test.rs but it's not really worth it in my opinion. It would keep us from tracking nameability though.

The tricky thing is that we don't want to expand all #[test]'s with use statements because this will make inner-fn tests produce spurious errors because:

fn foo() {
    #[test]
    fn bar(){}
}

would become

fn foo() {
    #[test]
    fn bar(){}
    use self::bar as bar_gensym;
}

which will fail.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jul 31, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/ae/3a/b30eb9b2e05d0fa102ad6f1544e10129de857a570ee25549665021fa7450/awscli-1.15.68-py2.py3-none-any.whl (1.3MB)
    0% |▎                               | 10kB 26.7MB/s eta 0:00:01
    1% |▌                               | 20kB 2.0MB/s eta 0:00:01
    2% |▊                               | 30kB 2.3MB/s eta 0:00:01
    3% |█                               | 40kB 2.1MB/s eta 0:00:01
---
[00:54:38]     |
[00:54:38] 552 |     fn test_multiple_int_constants() {
[00:54:38]     |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[00:54:38] 
[00:54:38] error: unused import: `test_op_i`
[00:54:38]    --> libterm/terminfo/parm.rs:558:8
[00:54:38] 558 |     fn test_op_i() {
[00:54:38]     |        ^^^^^^^^^
[00:54:38] 
[00:54:38] error: unused import: `test_param_stack_failure_conditions`

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@djrenren

This comment has been minimized.

Copy link
Contributor Author

djrenren commented Aug 1, 2018

@petrochenkov Any idea why those changes broke the build?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Aug 1, 2018

@djrenren
Are you sure it passed previously?
The generated imports do seem unused except in "the semi-pathological case where you have test functions referencing each other".

@djrenren

This comment has been minimized.

Copy link
Contributor Author

djrenren commented Aug 1, 2018

Yeah, the errors make sense, I'm just confused why it passed before...

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Aug 1, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:57:27] travis_fold:start:test_stage1-term
travis_time:start:test_stage1-term
Testing term stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:57:27]    Compiling term v0.0.0 (file:///checkout/src/libterm)
[00:57:27] error: unused import: `test_get_dbpath_for_term`
[00:57:27]   --> libterm/terminfo/searcher.rs:80:4
[00:57:27] 80 | fn test_get_dbpath_for_term() {
[00:57:27]    |    ^^^^^^^^^^^^^^^^^^^^^^^^
[00:57:27]    |
[00:57:27]    = note: `-D unused-imports` implied by `-D warnings`
[00:57:27]    = note: `-D unused-imports` implied by `-D warnings`
[00:57:27] 
[00:57:27] error: unused import: `test_veclens`
[00:57:27]    --> libterm/terminfo/parser/compiled.rs:351:8
[00:57:27] 351 |     fn test_veclens() {
[00:57:27]     |        ^^^^^^^^^^^^
[00:57:27] 
[00:57:27] 
[00:57:27] error: unused import: `test_basic_setabf`
[00:57:27]    --> libterm/terminfo/parm.rs:545:8
[00:57:27] 545 |     fn test_basic_setabf() {
[00:57:27]     |        ^^^^^^^^^^^^^^^^^
[00:57:27] 
[00:57:27] 
[00:57:27] error: unused import: `test_multiple_int_constants`
[00:57:27]    --> libterm/terminfo/parm.rs:552:8
[00:57:27] 552 |     fn test_multiple_int_constants() {
[00:57:27]     |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[00:57:27] 
[00:57:27] 
[00:57:27] error: unused import: `test_op_i`
[00:57:27]    --> libterm/terminfo/parm.rs:558:8
[00:57:27] 558 |     fn test_op_i() {
[00:57:27]     |        ^^^^^^^^^
[00:57:27] 
[00:57:27] error: unused import: `test_param_stack_failure_conditions`
---
[00:57:27]     |
[00:57:27] 618 |     fn test_push_bad_param() {
[00:57:27]     |        ^^^^^^^^^^^^^^^^^^^
[00:57:27] 
[00:57:27] error: unused import: `test_comparison_ops`
[00:57:27]    --> libterm/terminfo/parm.rs:623:8
[00:57:27] 623 |     fn test_comparison_ops() {
[00:57:27]     |        ^^^^^^^^^^^^^^^^^^^
[00:57:27] 
[00:57:27] 
[00:57:27] error: unused import: `test_conditionals`
[00:57:27]    --> libterm/terminfo/parm.rs:642:8
[00:57:27] 642 |     fn test_conditionals() {
[00:57:27]     |        ^^^^^^^^^^^^^^^^^
[00:57:27] 
[00:57:27] 
[00:57:27] error: unused import: `test_format`
[00:57:27]    --> libterm/terminfo/parm.rs:657:8
[00:57:27] 657 |     fn test_format() {
[00:57:27]     |        ^^^^^^^^^^^
[00:57:27] 
[00:57:28] error: aborting due to 10 previous errors
[00:57:28] error: aborting due to 10 previous errors
[00:57:28] 
[00:57:28] error: Could not compile `term`.
[00:57:28] To learn more, run the command again with --verbose.
[00:57:28] 
[00:57:28] 
[00:57:28] 
[00:57:28] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/libtest/Cargo.toml" "-p" "term" "--" "--quiet"
[00:57:28] 
[00:57:28] 
[00:57:28] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:57:28] Build completed unsuccessfully in 0:15:35
[00:57:28] Build completed unsuccessfully in 0:15:35
[00:57:28] Makefile:58: recipe for target 'check' failed
[00:57:28] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0770fe80
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@djrenren

This comment has been minimized.

Copy link
Contributor Author

djrenren commented Aug 1, 2018

@petrochenkov Should be all set now.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Aug 1, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 1, 2018

📌 Commit 77f9aca has been approved by petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 2, 2018

⌛️ Testing commit 77f9aca with merge 02a369a...

bors added a commit that referenced this pull request Aug 2, 2018

Auto merge of #52890 - djrenren:test-visibility, r=petrochenkov
Reexport tests without polluting namespaces

This should fix issue #52557.

Basically now we gensym a new name for the test function and reexport that.
That way the test function's reexport name can't conflict because it was impossible for the test author to write it down.
We then use a `use` statement to expose the original name using the original visibility.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 2, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 02a369a to master...

@bors bors merged commit 77f9aca into rust-lang:master Aug 2, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.