Skip to content

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Nov 4, 2021

This adds some more documentation for compiletest headers.

@ehuss ehuss force-pushed the compiletest-more-headers branch from 044fc38 to 8effd43 Compare November 4, 2021 18:13
Co-authored-by: pierwill <19642016+pierwill@users.noreply.github.com>
@ehuss
Copy link
Contributor Author

ehuss commented Nov 5, 2021

Thanks for taking a look! I have applied the suggestions.

@camelid camelid added the S-waiting-on-review Status: this PR is waiting for a reviewer to verify its content label Nov 8, 2021
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

I'm learning about most of these headers for the first time, so I didn't check for accuracy, but it looks good to me aside from that. I left a few nits.

Would it make sense to alphabetize this list to make it easier to skim?

Also, is there someone who can check to make sure the descriptions are accurate?

* `force-host` will force the test to build for the host platform instead of
the target. This is useful primarily for auxiliary proc-macros, which need
to be loaded by the host compiler.
* `pretty-mode` specifies the mode pretty-print tests should run in.
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, all tests >=check-pass are also run as pretty-print tests. Is that correct? If so, does pretty-mode affect those too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless I'm mistaken, the pretty-print checks that ran on all tests were removed several years ago. The only pretty-print tests now are those in the src/test/pretty directory.

There are a bunch of // pretty-expanded comment headers sprinkled across the testsuite, but those are vestigial.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I didn't realize that. Good to know, thanks.

@ehuss
Copy link
Contributor Author

ehuss commented Nov 19, 2021

Thanks for taking a look!

Would it make sense to alphabetize this list to make it easier to skim?

As mentioned on Zulip (here), I'd like to work on reorganizing the testing chapter. As part of that, I'd like to break this list up into different topics (for example, the pretty-print headers, debugger headers, incremental, etc.). I'd like to get this merged before I start on that.

@camelid
Copy link
Member

camelid commented Nov 19, 2021

Thanks for taking a look!

Sure, thanks for making this PR :)

As mentioned on Zulip (here), I'd like to work on reorganizing the testing chapter. As part of that, I'd like to break this list up into different topics (for example, the pretty-print headers, debugger headers, incremental, etc.). I'd like to get this merged before I start on that.

Sounds good.

@camelid
Copy link
Member

camelid commented Nov 19, 2021

@petrochenkov could you review this to check accuracy? I'm not super familiar with all the different headers.

@jyn514
Copy link
Member

jyn514 commented Nov 24, 2021

@camelid same here, I think this is good enough to merge and we can fix it later if necessary

@jyn514 jyn514 merged commit 8a817bc into rust-lang:master Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: this PR is waiting for a reviewer to verify its content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants