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

Issue 46841 Move tests from compile-fail into subdirectories #46915

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@technicalguy
Contributor

technicalguy commented Dec 21, 2017

Addresses issue #46841.

Created the following subdirectories:

associated-item
closures
coherence
const
diagnostics
empty-struct
issues
lint
macro
regions
traits
typing
variance

Additionally rename some tests:

renamed: unboxed-closer-non-implicit-copyable.rs -> closures/unboxed-closure-non-implicit-copyable.rs
renamed: issue32829.rs -> issues/issue-32829.rs
renamed: isssue-38821.rs -> issues/issue-38821.rs

Move some tests that reference auxiliary files

Move the following issues into the empty-struct and/or lint subdirectories
(I'm not sure if this is where they belong):
issues/issue-37026.rs -> empty-struct/issue-37026.rs
issues/issue-28075.rs -> lint/issue-28075-1.rs
issues/issue-28388-3.rs -> lint/issue-28388-3.rs

Move the following tests into the empty-struct, lint, or macro
subdirectories, even though they are not prefixed by those directory
names:
no-link.rs -> empty-struct/no-link.rs
enable-unstable-lib-feature.rs -> lint/enable-unstable-lib-feature.rs
missing-macro-use.rs -> macro/missing-macro-use.rs

r? @pnkfelix

technicalguy added some commits Dec 21, 2017

Move tests from compile-fail into subdirectories
Addresses issue #46841.

Created the following subdirectories:
associated-item
closures
coherence
const
diagnostics
issues
lint
macro
regions
traits
typing
variance

Additionally rename some tests:
renamed:    unboxed-closer-non-implicit-copyable.rs -> closures/unboxed-closure-non-implicit-copyable.rs
renamed:    issue32829.rs -> issues/issue-32829-1.rs
renamed:    isssue-38821.rs -> issues/issue-38821.rs
Move auxiliary files into new subdirectories
Move the following issues into the empty-struct and/or lint subdirectories
(I'm not sure if this is where they belong):
issues/issue-37026.rs -> empty-struct/issue-37026.rs
issues/issue-28075.rs -> lint/issue-28075.rs
issues/issue-28388-3.rs -> lint/issue-28388-3.rs

Move the following tests into the empty-struct, lint, or macro
subdirectories, even though they are not prefixed by those directory
names:
no-link.rs -> empty-struct/no-link.rs
enable-unstable-lib-feature.rs -> lint/enable-unstable-lib-feature.rs
missing-macro-use.rs -> macro/missing-macro-use.rs
@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Dec 21, 2017

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (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.

Collaborator

rust-highfive commented Dec 21, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (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.

@technicalguy

This comment has been minimized.

Show comment
Hide comment
@technicalguy

technicalguy Dec 21, 2017

Contributor

I figured I'd try to get my hands dirty with this E-easy issue....

I tried to put the most obvious "offenders" into subdirectories.

So this reduces the number of entries in src/test/compile-fail down to 983, but I wasn't sure how far you wanted to go. Presumably this wouldn't last very long...

I feel compelled to get it to the point that compiler-fail contains only subdirectories, and no .rs files. Although I don't know what all of the tests do, so it gets harder to categories them.

There were a few things I was unsure about:

  • Should the associated-types-... tests be moved into the associated-type folder? (rather than the associated-item folder?)
  • There's a bunch of tests that look like they could belong in multiple folders, e.g.
    • regions-steal-closure.rsregions folder, or closures folder?
    • associated-const-array-len.rsassociated-items folder, or const folder?
    • associated-const-upper-case-lint.rsassociated-items folder, or const folder, or even the lint folder?
    • unboxed-closure-sugar-region.rsclosures or regions?
    • etc...
Contributor

technicalguy commented Dec 21, 2017

I figured I'd try to get my hands dirty with this E-easy issue....

I tried to put the most obvious "offenders" into subdirectories.

So this reduces the number of entries in src/test/compile-fail down to 983, but I wasn't sure how far you wanted to go. Presumably this wouldn't last very long...

I feel compelled to get it to the point that compiler-fail contains only subdirectories, and no .rs files. Although I don't know what all of the tests do, so it gets harder to categories them.

There were a few things I was unsure about:

  • Should the associated-types-... tests be moved into the associated-type folder? (rather than the associated-item folder?)
  • There's a bunch of tests that look like they could belong in multiple folders, e.g.
    • regions-steal-closure.rsregions folder, or closures folder?
    • associated-const-array-len.rsassociated-items folder, or const folder?
    • associated-const-upper-case-lint.rsassociated-items folder, or const folder, or even the lint folder?
    • unboxed-closure-sugar-region.rsclosures or regions?
    • etc...

@pnkfelix pnkfelix added the T-compiler label Dec 21, 2017

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Dec 21, 2017

Member

@technicalguy thanks for the PR!

I want to double check with the rest of the compiler team about 1. whether they are on board with the refactoring into more directories, and if so, 2. whether to land it before or immediately after the next release cycle.

@rfcbot fcp merge

Member

pnkfelix commented Dec 21, 2017

@technicalguy thanks for the PR!

I want to double check with the rest of the compiler team about 1. whether they are on board with the refactoring into more directories, and if so, 2. whether to land it before or immediately after the next release cycle.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Dec 21, 2017

Team member @pnkfelix has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Dec 21, 2017

Team member @pnkfelix has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Dec 21, 2017

Member

(to be clear, my personal inclination is to merge this once everyone has their box checked off. If you are okay with the idea of doing this, but want to wait until after the next release, please do say so as via the rfcbot concern command. Likewise, if you think this is simply a bad idea, then also voice your concern in the same manner.)

Member

pnkfelix commented Dec 21, 2017

(to be clear, my personal inclination is to merge this once everyone has their box checked off. If you are okay with the idea of doing this, but want to wait until after the next release, please do say so as via the rfcbot concern command. Likewise, if you think this is simply a bad idea, then also voice your concern in the same manner.)

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Dec 21, 2017

Member

@technicalguy also, I know you put questions into your last comment. 😄

I don't have immediate answers for them. I see this as something that could be ongoing work; the first PR for it doesn't have to put everything into its final spot in one go. (Though of course we want to avoid have pointless file migrations, especially repeatedly for the same files.)

Member

pnkfelix commented Dec 21, 2017

@technicalguy also, I know you put questions into your last comment. 😄

I don't have immediate answers for them. I see this as something that could be ongoing work; the first PR for it doesn't have to put everything into its final spot in one go. (Though of course we want to avoid have pointless file migrations, especially repeatedly for the same files.)

@technicalguy

This comment has been minimized.

Show comment
Hide comment
@technicalguy

technicalguy Dec 22, 2017

Contributor

@pnkfelix Yeah, I figured these subdirectories would probably exist anyway, so this seems useful even if it's "incomplete". I guess the main concern as I see it is that as more subdirectories are created there might be more issue-#####.rs tests that need to be moved into other subdirectories – out of the issues subdirectory that this PR creates.

Contributor

technicalguy commented Dec 22, 2017

@pnkfelix Yeah, I figured these subdirectories would probably exist anyway, so this seems useful even if it's "incomplete". I guess the main concern as I see it is that as more subdirectories are created there might be more issue-#####.rs tests that need to be moved into other subdirectories – out of the issues subdirectory that this PR creates.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Dec 23, 2017

Contributor

One more easy target - moving all E0___.rs tests into error-index subdirectory.

error-index and issues subdirectories alone may be enough to fix the GitHub displaying issue mentioned in #46841.

In general, I'd prefer to not move files around like this, but rather group them while moving into UI directory instead, but I don't see anything sufficiently bad in this PR to block it.

Contributor

petrochenkov commented Dec 23, 2017

One more easy target - moving all E0___.rs tests into error-index subdirectory.

error-index and issues subdirectories alone may be enough to fix the GitHub displaying issue mentioned in #46841.

In general, I'd prefer to not move files around like this, but rather group them while moving into UI directory instead, but I don't see anything sufficiently bad in this PR to block it.

@technicalguy

This comment has been minimized.

Show comment
Hide comment
@technicalguy

technicalguy Dec 23, 2017

Contributor

@petrochenkov All the E0___.rs files have been moved into the diagnostics directory as suggested in #46841. Moving the E0___.rs files and the issue-_____.rs files was not enough to bring the compile-fail directory below 1000 entries, which is why I created the other directories mentioned above.

I don't have any preference about this – I'm just trying to get familiar with the rust development community, build process and the contribution process and this looked like an easy place to start. :)

Contributor

technicalguy commented Dec 23, 2017

@petrochenkov All the E0___.rs files have been moved into the diagnostics directory as suggested in #46841. Moving the E0___.rs files and the issue-_____.rs files was not enough to bring the compile-fail directory below 1000 entries, which is why I created the other directories mentioned above.

I don't have any preference about this – I'm just trying to get familiar with the rust development community, build process and the contribution process and this looked like an easy place to start. :)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Dec 23, 2017

Contributor
Contributor

nikomatsakis commented Dec 23, 2017

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Dec 24, 2017

Contributor

@technicalguy

All the E0___.rs files have been moved into the diagnostics directory as suggested in #46841.

Ah, I see. I thought I searched "E0" in GitHub diff and got nothing, but apparently something went wrong.
The directory name diagnostics is meaningless though, all tests in compile-fail test diagnostics. E0___.rs are for checking error code numbers in the error index specifically.

Contributor

petrochenkov commented Dec 24, 2017

@technicalguy

All the E0___.rs files have been moved into the diagnostics directory as suggested in #46841.

Ah, I see. I thought I searched "E0" in GitHub diff and got nothing, but apparently something went wrong.
The directory name diagnostics is meaningless though, all tests in compile-fail test diagnostics. E0___.rs are for checking error code numbers in the error index specifically.

@technicalguy

This comment has been minimized.

Show comment
Hide comment
@technicalguy

technicalguy Dec 24, 2017

Contributor

@petrochenkov I wondered if that is what they were for, perhaps something like error-codes would be a more suitable directory name.

Contributor

technicalguy commented Dec 24, 2017

@petrochenkov I wondered if that is what they were for, perhaps something like error-codes would be a more suitable directory name.

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Jan 8, 2018

Member

(just a status update: This is on the list of PR's for the compiler team to discuss in their weekly meeting. We didn't get to it last week because there was a lot of stuff to discuss w.r.t. regressions with the upcoming release. We should get to it this week.)

Member

pnkfelix commented Jan 8, 2018

(just a status update: This is on the list of PR's for the compiler team to discuss in their weekly meeting. We didn't get to it last week because there was a lot of stuff to discuss w.r.t. regressions with the upcoming release. We should get to it this week.)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 11, 2018

Contributor

We discussed in the @rust-lang/compiler meeting. Seems like most people are in favor of directories, though not without some hesitation. But one thing that came up was that it would be good to start establishing clearer conventions for test naming schemes (and documenting them).

I think the primary change we would like to ensure that we have a README file listing the directories and some description of what should be in each one.

(We can always change the groupings later.)

There was also some discussion of what to do about issue-1234.rs tests. My personal opinion is that they should go into the appropriate subdirectory, with other tests, but that for all the ones that already exist, we should just make a legacy-issues directory or something like that and stuff them in there.

Contributor

nikomatsakis commented Jan 11, 2018

We discussed in the @rust-lang/compiler meeting. Seems like most people are in favor of directories, though not without some hesitation. But one thing that came up was that it would be good to start establishing clearer conventions for test naming schemes (and documenting them).

I think the primary change we would like to ensure that we have a README file listing the directories and some description of what should be in each one.

(We can always change the groupings later.)

There was also some discussion of what to do about issue-1234.rs tests. My personal opinion is that they should go into the appropriate subdirectory, with other tests, but that for all the ones that already exist, we should just make a legacy-issues directory or something like that and stuff them in there.

@technicalguy

This comment has been minimized.

Show comment
Hide comment
@technicalguy

technicalguy Jan 11, 2018

Contributor

@nikomatsakis

There was also some discussion of what to do about issue-1234.rs tests. My personal opinion is that they should go into the appropriate subdirectory, with other tests, but that for all the ones that already exist, we should just make a legacy-issues directory or something like that and stuff them in there.

Just to be clear, you mean that no new tests should be called issue-#####.rs?

(Also, I don't mind if this pull request doesn't get used, it was just for practice. I created the directories based off of the filenames as suggested in #46841, hence names like empty-struct which has a fairly significant number of tests prefixed with that name.)

Contributor

technicalguy commented Jan 11, 2018

@nikomatsakis

There was also some discussion of what to do about issue-1234.rs tests. My personal opinion is that they should go into the appropriate subdirectory, with other tests, but that for all the ones that already exist, we should just make a legacy-issues directory or something like that and stuff them in there.

Just to be clear, you mean that no new tests should be called issue-#####.rs?

(Also, I don't mind if this pull request doesn't get used, it was just for practice. I created the directories based off of the filenames as suggested in #46841, hence names like empty-struct which has a fairly significant number of tests prefixed with that name.)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 11, 2018

Contributor

@technicalguy

you mean that no new tests should be called issue-#####.rs

I meant something slightly weaker: we should try to categorize issue-###.rs files into an appropriate subdirectory where possible.

I do think there's a case to be made that more descriptive filenames ought to be preferred, though. But I've definitely encountered times when there is just some wacky regression test targeting what seems to be a very narrow scenario, and it's hard to give it a useful name. (I've also sometimes found myself adding both an artificial regression test that I made and the original test, since the original seemed to exercise things in a more complex way, but I wanted a narrower test too.)

Contributor

nikomatsakis commented Jan 11, 2018

@technicalguy

you mean that no new tests should be called issue-#####.rs

I meant something slightly weaker: we should try to categorize issue-###.rs files into an appropriate subdirectory where possible.

I do think there's a case to be made that more descriptive filenames ought to be preferred, though. But I've definitely encountered times when there is just some wacky regression test targeting what seems to be a very narrow scenario, and it's hard to give it a useful name. (I've also sometimes found myself adding both an artificial regression test that I made and the original test, since the original seemed to exercise things in a more complex way, but I wanted a narrower test too.)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 18, 2018

Contributor

@technicalguy I think there is general agreement that, if we make some guidelines, we can go ahead and introduce directories. Do you want to try and rebse this PR? Alternatively, maybe we should close for now and instead write up the guidelines first, and then we can do the migration gradually?

(I think I favor the latter)

Contributor

nikomatsakis commented Jan 18, 2018

@technicalguy I think there is general agreement that, if we make some guidelines, we can go ahead and introduce directories. Do you want to try and rebse this PR? Alternatively, maybe we should close for now and instead write up the guidelines first, and then we can do the migration gradually?

(I think I favor the latter)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 18, 2018

Contributor

@technicalguy I'd be happy to work with you on guidelines

Contributor

nikomatsakis commented Jan 18, 2018

@technicalguy I'd be happy to work with you on guidelines

@technicalguy

This comment has been minimized.

Show comment
Hide comment
@technicalguy

technicalguy Jan 18, 2018

Contributor

write up the guidelines first

Yeah, that sounds like a better idea. I will not be able to work on this for a while unfortunately.

Contributor

technicalguy commented Jan 18, 2018

write up the guidelines first

Yeah, that sounds like a better idea. I will not be able to work on this for a while unfortunately.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jan 18, 2018

Contributor

OK, going to close this issue for now then. @technicalguy I'd love to work with you more on it though =)

Contributor

nikomatsakis commented Jan 18, 2018

OK, going to close this issue for now then. @technicalguy I'd love to work with you more on it though =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment