Skip to content

Delete low-value PAL test #48932

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
jkotas merged 1 commit into from
Mar 2, 2021
Merged

Delete low-value PAL test #48932

jkotas merged 1 commit into from
Mar 2, 2021

Conversation

@jkotas
Copy link
Member

@jkotas jkotas commented Mar 2, 2021

This partially reverts commit 77c939c.

Fixes #48496

@jkotas
Delete low-value PAL test
eb74f23 
This partially reverts commit 77c939c.

Fixes dotnet#48496
@ghost ghost added the area-PAL-coreclr label Mar 2, 2021
@jkotas jkotas requested a review from BruceForstall March 2, 2021 01:09
@jkotas jkotas mentioned this pull request Mar 2, 2021
Closed
@danmoseley
Copy link
Member

danmoseley commented Mar 2, 2021

Without this test, couldn't I replace the body of SetThreadDescription(..) in the PAL with return 0; and all our tests would pass? This is the only test that verifies we actually set the native thread name.

Why not keep this test, which is working fine, and make the fix suggested in #48496?

BruceForstall
BruceForstall approved these changes Mar 2, 2021
View reviewed changes
Copy link
Contributor

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

I'm ok with this if you think it's the right thing to do.

src/coreclr/pal/tests/palsuite/compilableTests.txt
threading/ResetEvent/test3/paltest_resetevent_test3
threading/ResetEvent/test4/paltest_resetevent_test4
threading/ResumeThread/test1/paltest_resumethread_test1
threading/SwitchToThread/test1/paltest_switchtothread_test1
Copy link
Contributor

@BruceForstall BruceForstall Mar 2, 2021

Choose a reason for hiding this comment

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

Is this related?

Copy link
Member Author

@jkotas jkotas Mar 2, 2021

Choose a reason for hiding this comment

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

This line is redundant - the same line is ~10 lines below already. I think this was just an incorrectly resolved conflict in the original PR.

@jkotas
Copy link
Member Author

jkotas commented Mar 2, 2021

Without this test, couldn't I replace the body of SetThreadDescription(..) in the PAL with return 0; and all our tests would pass?

Yep. There are many other ways how one can break the connection between Thread.Name managed API and the OS name that this test is not catching either. It is what makes this test low-value.

If we wanted to have actual testing here, we would be testing that setting Thread.Name is actually going to show up in the diagnostic tools.

Why not keep this test, which is working fine, and make the fix suggested in #48496?

It is not worth my time to work on tweaking this to make it pass. If you would like to work on this, feel free to close this PR and submit alternative.

@danmoseley
Copy link
Member

danmoseley commented Mar 2, 2021

Fair enough

@jkotas
Copy link
Member Author

jkotas commented Mar 2, 2021

Also, setting of the OS thread name would be ideally shared between runtimes, via the shared CoreLib and System.Native shim.

@jkotas jkotas merged commit 3bb4ce4 into dotnet:main Mar 2, 2021
@jkotas jkotas deleted the issue-47465 branch March 2, 2021 17:46
@ghost ghost locked as resolved and limited conversation to collaborators Apr 1, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@BruceForstall BruceForstall BruceForstall approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-PAL-coreclr

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

Test failed : threading.SetThreadDescription.test1

4 participants

@jkotas @danmoseley @BruceForstall @karelz