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

gensym each test re-export module individually #16892

Merged
merged 3 commits into from
Sep 3, 2014
Merged

gensym each test re-export module individually #16892

merged 3 commits into from
Sep 3, 2014

Conversation

andrew-d
Copy link
Contributor

Fixes #16597

I'm not 100% sure this is the correct way to handle this - but I wasn't able to find a better way without doing way more refactoring of the code that I was comfortable with. Comments and criticism are appreciated 😄

cx.sess.span_bug(
DUMMY_SP,
"expected to find top-level re-export name, but found None"
);
Copy link
Member

Choose a reason for hiding this comment

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

If this just uses a dummy span, it should call bug instead of span_bug. It also looks like some of this module is conditionally executed, so could this bug possibly be triggered on something like an empty file?

@alexcrichton
Copy link
Member

Could you add a test for #16597 as well?

@andrew-d
Copy link
Contributor Author

Added, and an empty test too. The tests pass make check locally.

@alexcrichton
Copy link
Member

cc @sfackler, the call to bug looks odd to me, but you may know more about whether it could be triggered or not.

@andrew-d
Copy link
Contributor Author

I don't think it's possible for the call to bug() to trigger. From what I can see, the path is reached like so:

TestHarnessGenerator::fold_crate
             |
       mk_test_module
             |
          mk_tests
             |
       mk_test_descs
             |
   mk_test_desc_and_fn_rec

And the only way that mk_test_desc_and_fn_rec is called is if there's more than one item in cx.testfns. The only way THAT happens is if the Folder hits the test function here. If that function is hit, then the conditional here can't be false, and we should always hit the assignment.

All that being said: another pair of eyes would be welcome 😆

@andrew-d
Copy link
Contributor Author

andrew-d commented Sep 1, 2014

@alexcrichton @sfackler Looks like this error is because the pretty-printer doesn't like gensym'd things? I can fix this by // ignore-pretty in the test, or by switching to another method of generating module names - for example, an AtomicUint and names of the form __test_reexports_1234. Thoughts?

@sfackler
Copy link
Member

sfackler commented Sep 2, 2014

// ignore-pretty seems reasonable to me. It's something that should really be fixed in the pretty printer itself at some point.

@andrew-d
Copy link
Contributor Author

andrew-d commented Sep 2, 2014

@sfackler Done :-) Can I get another r+?

bors added a commit that referenced this pull request Sep 3, 2014
…ckler

Fixes #16597

I'm not 100% sure this is the correct way to handle this - but I wasn't able to find a better way without doing way more refactoring of the code that I was comfortable with.  Comments and criticism are appreciated 😄
@bors bors closed this Sep 3, 2014
@bors bors merged commit 9374d50 into rust-lang:master Sep 3, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 31, 2024
internal: Don't eagerly try to read crate root file contents before VFS

Fixes rust-lang/rust-analyzer#8623
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing error when using import globbing in test module
4 participants