Skip to content

Fix TFM for function pointer expected output tests #69506

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 22, 2023
Merged

Fix TFM for function pointer expected output tests #69506

merged 1 commit into from
Aug 22, 2023

Conversation

@jjonescz
Copy link
Member

@jjonescz jjonescz commented Aug 15, 2023 •
edited
Loading

This is a follow-up on #68212.

The tests didn't work for me locally on Windows with TFM net7.0.

@ghost ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 15, 2023
@jjonescz jjonescz changed the title Fix fnptr expected output condition on Windows Fix TFM for function pointer expected output tests Aug 15, 2023
@jjonescz jjonescz marked this pull request as ready for review August 15, 2023 14:46
@jjonescz jjonescz requested a review from a team as a code owner August 15, 2023 14:46
jcouv
jcouv reviewed Aug 21, 2023
View reviewed changes
src/Compilers/CSharp/Test/Emit/Microsoft.CodeAnalysis.CSharp.Emit.UnitTests.csproj
<OutputType>Library</OutputType>
<RootNamespace>Microsoft.CodeAnalysis.CSharp.UnitTests</RootNamespace>
<TargetFrameworks>net7.0;net472</TargetFrameworks>
<TargetFrameworks>net8.0;net472</TargetFrameworks>
Copy link
Member

@jcouv jcouv Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like some other projects still reference TFM net7.0 as well. For example EndToEnd and IOperations unittests. Should they be updated too? I don't know if it's intentional. May be good to check with Jared.

Copy link
Member

@jcouv jcouv Aug 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjonescz @jaredpar Should we update all tests to use net8.0 TFM, instead of piecemeal?

Copy link
Member

@jaredpar jaredpar Aug 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable at this point

Copy link
Member Author

@jjonescz jjonescz Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#69669

jcouv
jcouv approved these changes Aug 21, 2023
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 1)

@jcouv jcouv self-assigned this Aug 21, 2023
@333fred
Copy link
Member

333fred commented Aug 22, 2023

I don't understand how this is related to function pointers or why this would fix anything running locally for you. Can you clarify?

@jjonescz
Copy link
Member Author

jjonescz commented Aug 22, 2023

I don't understand how this is related to function pointers or why this would fix anything running locally for you. Can you clarify?

I didn't figure out the exact reason, but simply having net7.0 in those test projects makes exactly the tests changed in #68212 print System.IntPtr instead of System.Void() for me locally, whereas updating to net8.0 works as expected - I guess the reason has something to do with the fact that the behavior changed between .NET 7 and .NET 8 runtimes.

@jjonescz jjonescz requested a review from 333fred August 22, 2023 09:06
@333fred
Copy link
Member

333fred commented Aug 22, 2023

Thanks, that clarifies it for me.

@jjonescz jjonescz merged commit 9742715 into dotnet:main Aug 22, 2023
@jjonescz jjonescz deleted the 68212-FnptrTests branch August 22, 2023 18:48
@ghost ghost added this to the Next milestone Aug 22, 2023
@jjonescz jjonescz mentioned this pull request Aug 23, 2023
Merged
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaredpar jaredpar jaredpar left review comments

@jcouv jcouv jcouv approved these changes

@333fred 333fred 333fred approved these changes

Assignees

@jcouv jcouv

Labels

Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

Milestone

17.8 P2

Development

Successfully merging this pull request may close these issues.

5 participants

@jjonescz @333fred @jaredpar @jcouv @dibarbet