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

superpmi unit test dependencies build issue #76421

Closed
Tracked by #79018
BruceForstall opened this issue Sep 30, 2022 · 3 comments · Fixed by #84374
Closed
Tracked by #79018

superpmi unit test dependencies build issue #76421

BruceForstall opened this issue Sep 30, 2022 · 3 comments · Fixed by #84374

Comments

@BruceForstall
Copy link
Member

The superpmi unit test, src/tests/JIT/superpmi, has a project reference to two other projects (currently):

Performance\CodeQuality\Bytemark\Bytemark.csproj
Methodical\fp\exgen\10w5d_cs_do.csproj

These get built and copied into the superpmi unit test directory. The superpmi unit test then invokes them while doing a superpmi collection.

As part of the new test execution system, 10w5d_cs_do was converted to not build as standalone, and so it no longer has a .cmd/.sh wrapper script. As a result, the superpmi unit test driver can't execute it.

We need a way to build 10w5d_cs_do (and/or other tests) for use with the superpmi unit test, such that they are built with BuildAsStandalone=true, or some other mechanism.

This change didn't cause a failure in the superpmi unit test itself due to insufficient error checking, fixed by #76411.

@ghost
Copy link

ghost commented Sep 30, 2022

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

Issue Details

The superpmi unit test, src/tests/JIT/superpmi, has a project reference to two other projects (currently):

Performance\CodeQuality\Bytemark\Bytemark.csproj
Methodical\fp\exgen\10w5d_cs_do.csproj

These get built and copied into the superpmi unit test directory. The superpmi unit test then invokes them while doing a superpmi collection.

As part of the new test execution system, 10w5d_cs_do was converted to not build as standalone, and so it no longer has a .cmd/.sh wrapper script. As a result, the superpmi unit test driver can't execute it.

We need a way to build 10w5d_cs_do (and/or other tests) for use with the superpmi unit test, such that they are built with BuildAsStandalone=true, or some other mechanism.

This change didn't cause a failure in the superpmi unit test itself due to insufficient error checking, fixed by #76411.

Author: BruceForstall
Assignees: BruceForstall
Labels:

area-Infrastructure-coreclr

Milestone: 8.0.0

BruceForstall added a commit to BruceForstall/runtime that referenced this issue Oct 1, 2022
Add CscBench to get better C# coverage.

Disable 10w5d_cs_do.csproj due to dotnet#76421
BruceForstall added a commit that referenced this issue Oct 1, 2022
* Fix SuperPMI unit test error checking

We were ignoring test failures, and even ignoring if the test
we wanted to run under collection actually existed. This means
that at some point during recent test reconfigurations, we
stopped building 10w5d_cs_do.cmd/sh but we didn't notice. Because
Bytemark kept being built as before, we got at least some .mc
files.

Add more checking that the programs we want to run exist, and
that the tests we invoke pass.

* Adjust superpmi unit test collection tests

Add CscBench to get better C# coverage.

Disable 10w5d_cs_do.csproj due to #76421
@BruceForstall
Copy link
Member Author

@BrianBohe @markples fyi, this will be an issue when converting more of the test tree to the new merged model.

@BruceForstall BruceForstall removed their assignment Feb 6, 2023
@markples
Copy link
Member

markples commented Feb 6, 2023

For now, I'm pretty sure that you can set <RequiresProcessIsolation>true</RequiresProcessIsolation> on it. I'm not sure if setting BuildAsStandalone (which would be a better self-documenting name to use) is good enough. Either way, pulling a few tests out of the merged groups will be fine. If we need a substantial fraction of the tests changed this way, then we'll need something more clever.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 5, 2023
markples added a commit that referenced this issue Apr 6, 2023
This should fix test builds with `set BuildAsStandalone=true`.

- [Remove](5f14f05) unconditional `<BuildAsStandalone>false</BuildAsStandalone>` from various HardwareIntrinsics tests. This is already the default via Directory.Build.props but interferes with `set BuildAsStandalone=true`.  (nit - also remove an empty `PropertyGroup` from GitHub_43569.csproj)
- [Remove `Main` methods](0a886f8). We might be able to pursue a strategy where the test author provides a `Main(args)` method for local custom execution. The test would either need a separate parameterless [Fact] method or a [Theory] plus data for `Main` for merged group to execute it. However, a RequiresProcessIsolation/BuildAsStandalone build would ignore that metadata, so I'd like to avoid that complexity. The Main methods here set a bool or parse an int, which can be added locally to any test as needed.
- [Add comment](e79a10c) to Directory.Merged.props (addressing feedback on #83462)
- [Add the Methodical test back into the superpmi test](7c61de8) using RequiresProcessIsolation

Fixes #76421
Fixes #81984
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 6, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants