Skip to content

Conversation

@MiYanni
Copy link
Member

@MiYanni MiYanni commented Nov 28, 2023

Summary

I started to work on the dotnet/installer merge and realized how unconventional src\Tests was. Most repo structure conventions in the last decade have a dedicated test folder at the root of the repo, since src is generally for product source code only. So, I moved src\Tests to test and adjusted what was necessary to build such as relative paths. This PR doesn't change anything about the output structure (that within artifacts). This is only the structure of the files within the repo itself.

The dotnet/installer repo also has a test folder at the root. I believe many of the older repos (runtime, roslyn, etc.) don't have a test folder at the root as this kind of paradigm wasn't established. The test folder convention just helps with separation-of-concerns for how to scaffold the contents of the repo. It has no functional changes.

Reference

There are many other sources on this information. Additionally, one of the repos that was merged into this, dotnet/cli, also had a test folder. I can find more information if need be.

@ghost ghost added Area-dotnet new the item is related to dotnet new command untriaged Request triage from a team member labels Nov 28, 2023
@MiYanni MiYanni requested review from a team, baronfel, dsplaisted and joeloff November 28, 2023 23:26
@v-wuzhai
Copy link
Contributor

It looks like there are still some test files under src/Tests.

@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 29, 2023

I'm not sure whether I'm fan of the singular word test. Did you consider the plural form tests? That's what most repos use in their subfolders.

Aside from that, I see the model that we have in the libraries part of dotnet/runtime as the preferred one over a dedicated tests folder. In libraries we have have the following folder structure per component:

Component\
          gen (optional)
          ref (optional)
          src
          tests

Tests often depend on shared sources or other parts from the src directory, i.e. referencing the source project via a P2P. Grouping them together per component has worked really well in runtime.

FWIW I was planning to move the APICompat and GenAPI test projects into the Compatibility folder, next to the source project folders here in this repo.

@Forgind
Copy link
Contributor

Forgind commented Nov 29, 2023

The changes look good to me. I only hesitate to approve because, like ViktorHofer, I'm not sure it's really better to have a dedicated test folder. I'm assuming that if we take this change, no similar changes would occur in the roslyn, runtime, msbuild, etc. repos to put tests in a dedicated test folder, and I'd rather align with other dotnet repos (particularly the 'flagship' dotnet repos) than with random other repos.

I agree that test code isn't intended to ship, but I'd argue it's an important part of the product, and good tests should not be treated as an afterthought. Using MSBuild as an example, there's a lot of source code in the src directory that doesn't normally ship! It's there for legacy reasons. (And if you can remove it without breaking people, you will make rainersigwald very happy.)

@MiYanni
Copy link
Member Author

MiYanni commented Nov 29, 2023

@v-wuzhai

It looks like there are still some test files under src/Tests.

That happened when I merged main back into the branch. They were new files that I missed. They are moved now.

@MiYanni
Copy link
Member Author

MiYanni commented Nov 29, 2023

@ViktorHofer

I'm not sure whether I'm fan of the singular word test. Did you consider the plural form tests? That's what most repos use in their subfolders.

I've seen both test and tests used. src itself is singular, even though it is an abbreviation. I didn't believe it makes any discernable difference in understanding if it is singular or plural.

I don't know why the standard uses plural for some and singular for others. Take the PowerShell repo for example. It has singular test, but other things like tools, docs, demos, and assets are all plural (which is the norm). I'm totally fine with this repo using either test or tests. Makes no difference to me.

Aside from that, I see the model that we have in the libraries part of dotnet/runtime as the preferred one over a dedicated tests folder. In libraries we have have the following folder structure per component:

Component\
          gen (optional)
          ref (optional)
          src
          tests

Tests often depend on shared sources or other parts from the src directory, i.e. referencing the source project via a P2P. Grouping them together per component has worked really well in runtime.

Azure PowerShell had a similar concept per Azure service:

[ServiceName]\
    [ServiceName]          (primary project)
    [ServiceName].Test     (test project)
    [ServiceName].AutoRest (auto-generated surface)

I think something like this would be great for this repo. However, I'm not looking to restructure the entire repo at the moment. I'm just going to be moving the dotnet/installer repo contents into this one. That repo uses a root test folder.

FWIW I was planning to move the APICompat and GenAPI test projects into the Compatibility folder, next to the source project folders here in this repo.

That would work great for a component-based folder structure. This repo kinda has that but a bunch of things aren't set up that way. Based on this discussion, though, I really think segregating the repo by components is likely the best solution.

@MiYanni
Copy link
Member Author

MiYanni commented Nov 29, 2023

@Forgind

The changes look good to me. I only hesitate to approve because, like ViktorHofer, I'm not sure it's really better to have a dedicated test folder. I'm assuming that if we take this change, no similar changes would occur in the roslyn, runtime, msbuild, etc. repos to put tests in a dedicated test folder, and I'd rather align with other dotnet repos (particularly the 'flagship' dotnet repos) than with random other repos.

This (root-level test folder) is aligning with the standard of the open-source community. Unfortunately, I don't know the cause of the src/Tests convention used by some of the other dotnet repos. But they are the outlier, not the norm. My discussion with Viktor was making this repo to use more of a componentized model, which is considered the "multi-product" or "monorepo" style. I'd prefer that over either src/Tests or test. It helps to divide responsibilities and units of work.

I agree that test code isn't intended to ship, but I'd argue it's an important part of the product, and good tests should not be treated as an afterthought. Using MSBuild as an example, there's a lot of source code in the src directory that doesn't normally ship! It's there for legacy reasons. (And if you can remove it without breaking people, you will make rainersigwald very happy.)

Tests are a very important part of the product. They are so important, I feel they should have their own root-level folder. 😉 As it stands, this repo has a half-single product, half-componentized folder structure, which is confusing. Even the layout of the test projects/folders don't make a lot of sense to me and involve a lot of weird linking to source files (meaning, the folder structure doesn't relate to the Solution Explorer structure when viewing the project in VS). I'd love to restructure the repo to be a bit more logical, but that's not my focus at this point. 😢

@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 30, 2023

I'm fine with this if it helps to merge installer and sdk together (if that's the goal). Please file a follow-up issue to track the migration to a componentized folder structure.

@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Nov 30, 2023

In other repositories, I often use commands like git grep -e foo -- ":(exclude)*/Tests/*". I hope dotnet/sdk will be consistent within the repo, so that one exclusion is enough.

@Forgind
Copy link
Contributor

Forgind commented Nov 30, 2023

In other repositories, I often use commands like git grep -e foo -- ":(exclude)*/Tests/*". I hope dotnet/sdk will be consistent within the repo, so that one exclusion is enough.

This is a great point in favor of at least making it Tests instead of test...though I'm not 100% sure whether * can match "". A root-level directory called test with a subdirectory called Tests would definitely work for that but would not look at all good 😆

@KalleOlaviNiemitalo
Copy link
Contributor

I mean, it's fine if dotnet/sdk has test/. But please don't put both test/ and */tests/ in the same repo.

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

Labels

Area-dotnet new the item is related to dotnet new command untriaged Request triage from a team member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants