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

Explicitly mark tests with CLRTestKind=SharedLibrary #61235

Merged
merged 3 commits into from
Nov 11, 2021

Conversation

trylek
Copy link
Member

@trylek trylek commented Nov 5, 2021

Previously the Directory.Build.targets script used to automatically
infer that OutputType=Library without a CLRTestKind implies
SharedLibrary. This is however hard to consolidate with the planned
test merging - as the SDK scripts set OutputType=Library by default,
we need the combination Library+(implicit)BuildAndRun to indicate
the "new-style" [Fact]-based tests. For this reason I propose to
remove this automatic inference and manually fix the handful of tests
that are missing an explicit CLRTestKind=SharedLibrary property.

In light of this description we can theoretically remove the
OutputType=Library specification from all test projects but even if
we decide to do that, I believe it will be easier to do it as a
separate mechanical change, not as part of this relatively small
change that has a different purpose. Additionally in the one case
of the GitHub_22583 regression test, I removed the explicit setting
of GenerateRunScript=false because that's the default.

Thanks

Tomas

Contributes to: #54512

/cc @dotnet/runtime-infrastructure

@ghost
Copy link

ghost commented Nov 5, 2021

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

Previously the Directory.Build.targets script used to automatically
infer that OutputType=Library without a CLRTestKind implies
SharedLibrary. This is however hard to consolidate with the planned
test merging - as the SDK scripts set OutputType=Library by default,
we need the combination Library+(implicit)BuildAndRun to indicate
the "new-style" [Fact]-based tests. For this reason I propose to
remove this automatic inference and manually fix the handful of tests
that are missing an explicit CLRTestKind=SharedLibrary property.

In light of this description we can theoretically remove the
OutputType=Library specification from all test projects but even if
we decide to do that, I believe it will be easier to do it as a
separate mechanical change, not as part of this relatively small
change that has a different purpose. Additionally in the one case
of the GitHub_22583 regression test, I removed the explicit setting
of GenerateRunScript=false because that's the default.

Thanks

Tomas

Contributes to: #54512

/cc @dotnet/runtime-infrastructure

Author: trylek
Assignees: -
Labels:

area-Infrastructure-coreclr

Milestone: -

@trylek
Copy link
Member Author

trylek commented Nov 8, 2021

OK, I believe I'm finally out of the woods w.r.t. this change, the formatting failure on Windows x64 seems infrastructural and the GC hole in test45929 is known, I sent out a PR yesterday to temporarily block the test out in issues.targets to stop it from plaguing our outerloop runs; I'm retrying both legs now, please review when you have a chance.

Thanks

Tomas

@trylek trylek closed this Nov 9, 2021
@trylek trylek reopened this Nov 9, 2021
@trylek
Copy link
Member Author

trylek commented Nov 9, 2021

Apparently the latest runs have hit #57621. I'm going to push another commit adding it to the issues.targets file and re-run the outerloop unless @kouvel has other suggestions.

@kouvel
Copy link
Member

kouvel commented Nov 9, 2021

Sounds good to me @trylek, I haven't looked at that test yet

Previously the Directory.Build.targets script used to automatically
infer that OutputType=Library without a CLRTestKind implies
SharedLibrary. This is however hard to consolidate with the planned
test merging - as the SDK script set OutputType=Library by default,
we need the combination Library+(implicit)BuildAndRun to indicate
the "new-style" [Fact]-based tests. For this reason I propose to
remove this automatic inference and manually fix the handful of tests
that are missing an explicit CLRTestKind=SharedLibrary property.
In light of this description we can theoretically remove the
OutputType=Library specification from all test projects but even if
we decide to do that, I believe it will be easier to do that as a
separate mechanical change, not as part of this relatively small
change that has a different purpose. Additionally in the one case
of the GitHub_22583 regression test, I removed the explicit setting
of GenerateRunScript=false because that's the default.

Thanks

Tomas
I believe this was a pre-existing bug - previously, with the
special clause regarding SharedLibrary, the test just got silently
skipped because it was considered to be a shared library.

Thanks

Tomas
@trylek
Copy link
Member Author

trylek commented Nov 11, 2021

The outerloop and PR run have passed, I don't see how library test runs could be affected by my change only targeting CoreCLR tests and I believe the one Mono failure to be unrelated to my change, merging in.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants