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

July infra rollout: move tests shared between coreclr and mono from src/coreclr/tests/src to src/tests #37440

Closed
trylek opened this issue Jun 4, 2020 · 13 comments · Fixed by #38058

Comments

@trylek
Copy link
Member

trylek commented Jun 4, 2020

As what was previously the "CoreCLR test suite" is now shared with mono, it no longer makes sense for it to sit under src/coreclr/tests in the runtime tree. The purpose of this task is to move the tests into a new common folder under the tree and modify the paths in all scripts and pipelines so that they continue running after the change. This task doesn't address any changes to the underlying harness used to run the tests.

Current root location of CoreCLR test source code and scripts:

src/coreclr/tests/src

Proposed new location for the test root:

src/tests

/cc: @dotnet/runtime-infrastructure

@trylek trylek self-assigned this Jun 4, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jun 4, 2020
@naricc
Copy link
Contributor

naricc commented Jun 4, 2020

Is this just going to involve a lot of updating paths? Or are there more substantive changes involved?

@ViktorHofer
Copy link
Member

@jkotas what's your stance on the new path? Are you in favor of tests/runtime or something under src like src/tests/runtime?

@trylek
Copy link
Member Author

trylek commented Jun 4, 2020

@naricc - I believe we should just "update the paths" in this one step and tackle any deeper semantic changes regarding test execution later. I think that even in its limited extent this will be a bit of fun :-).

@jashook jashook removed the untriaged New issue has not been triaged by the area owner label Jun 4, 2020
@SamMonoRT
Copy link
Member

@jkotas
Copy link
Member

jkotas commented Jun 4, 2020

Are you in favor of tests/runtime or something under src like src/tests/runtime?

My thoughts:

  • For better or worse, we have the extra src level at the root and all sources under it. We should be consistent with it.
    • If we believe that this extra src level is not useful (e.g. it looks odd to have two src in the path like src\libraries\System.AppContext\src), we should get rid of it from everywhere and have e.g. libraries as the top level directory.
  • What other directories would we expect to have under tests? I think it is hard to tell where the "runtime" ends and the "other" stuff starts. In fact, the whole repo is the runtime. It may be better to make this a general purpose directory for all non-API tests, and have area-specific subdirectories under it. I think we can agree on what are the GC-specific or interop-specific tests.

So my pick would be src/tests with area-specific subdirectories like src/tests/Interop, src/tests/GC, etc.

Separately, it would be great to make it easier to write XUnit-like tests under this sub-tree. There is a hand-rolled xunit runner that is used to author tests like https://github.com/dotnet/runtime/blob/master/src/coreclr/tests/src/Interop/ICustomMarshaler/Primitives/ICustomMarshaler.cs#L46 . It works, but I think there are opportunities to make it work better.

@akoeplinger
Copy link
Member

It feels a bit confusing to me that a src/tests folder wouldn't contain all the tests including libraries tests, maybe something like src/runtime-tests is better?

Or at the very least explain in the src/tests/README.md that it doesn't include libraries tests and where to find them.

@jkotas
Copy link
Member

jkotas commented Jun 5, 2020

src/tests/README.md is certainly a good idea.

I do not think runtime-tests helps with clarity. nonapi-tests or nonlibraries-tests would be better, but it is a mouthful, so I would rather go with just tests.

@trylek
Copy link
Member Author

trylek commented Jun 5, 2020

Thanks everyone for your initial feedback. I like @jkotas' counterproposal to my original suggestion and I'm completely fine going with it. According to my understanding of his suggestion, we should basically map today subfolders of src/coreclr/tests/src under src/tests, e.g. src/coreclr/tests/src/JIT will become src/tests/JIT.

As a first cut we can just move the test source code and keep the scripts under src/coreclr/tests/src and src/coreclr/tests/src/Common in their current locations; we can iterate on their relocation and / or refactoring independently after the tests themselves have been moved.

For now I treat this as the champion proposal. If anyone believes they have a more reasonable suggestion, feel free to chime in and we can do some voting via emojis :-).

@trylek
Copy link
Member Author

trylek commented Jun 6, 2020

As a slight update I'm starting to think that it would be useful to have another intermediate folder level under src/tests, something like src/tests/runtime, the rationale being that CoreCLR test processing is based on scanning a folder subtree; once we start adding e.g. installer tests to the folder, it's going to be much easier to filter out those tests that are supposed to be run via the CoreCLR / Mono "runtime test harness" no matter how the meaning of this term is going to evolve.

@jkotas
Copy link
Member

jkotas commented Jun 6, 2020

I do not see a fundamental difference between e.g. interop tests and installer tests. Why can't all be discovered using the same folder scanning?

@trylek
Copy link
Member Author

trylek commented Jun 7, 2020

Well, at the very least they use different test harnesses today. I'm not against unifying them over time but doing all that in one fell swoop is beyond my imagination logistically. I may be mistaken but it also seems to me that the audience and purposes of runtime vs. library vs. installer tests are often different, making these test groups worth being observable by any runtime contributor.

@jkotas
Copy link
Member

jkotas commented Jun 7, 2020

Installer Host is a well defined narrow area. Having src/tests/HostActivation or src/tests/HostModel would be perfectly fine with me if we wanted to move the current installer tests there.

Runtime is poorly defined broad area. It is why I dislike src/tests/runtime.

The CoreCLR tests have very few requirements. It is just an entrypoint .exe and error code that it is expected to return, the rest is up to you. You can fit any test into this. The feature specific tests often customize it today. For example, compare these feature area specific tests:

Each of these have their own feature-specific test harness and shape of tests that is not one like other. Installer tests can be on this plan too if we choose to move them here.

@BruceForstall
Copy link
Member

@trylek You might want to update the initial message in this issue to be up-to-date with the current plan.

@trylek trylek changed the title Folder structure: move tests shared between coreclr and mono out of src/coreclr/tests July infra rollout: move tests shared between coreclr and mono from src/coreclr/tests/src to src/tests Jun 17, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.