Exception format cleanup subset by danmoseley · Pull Request #25185 · dotnet/coreclr

danmoseley · 2019-06-15T20:14:25Z

Subset of #25045 that seems reasonable to put into 3.0 as it's less likely to break output-parsing.

Remove newline within AE message.
Put newlines before each inner exception (this puts ---> somemessage on their own lines)
Stop duplicating one of the inner exceptoins in the AgE.ToString().

Deferred:

Indenting
Removing <--- and the various horizontal markers.

Test program original output and new output (file/line numbers aren't resolving for some reason to do with Debug runtime build)

src/System.Private.CoreLib/shared/System/Exception.cs

src/System.Private.CoreLib/shared/System/AggregateException.cs

src/System.Private.CoreLib/Resources/Strings.resx

tests/CoreFX/CoreFX.issues.rsp

jkotas · 2019-06-17T05:50:53Z

Looks good to me otherwise. Thank you!

danmoseley · 2019-06-18T18:13:16Z

I want to test this one more time before merging

danmoseley · 2019-06-19T08:28:23Z

Linux ARM tests failed twice with errors like

  Starting:    baseservices.threading.XUnitWrapper
    baseservices/threading/coverage/OSThreadId/osthreadid/osthreadid.sh [FAIL]
      Test Infrastructure Failure: Access to the path '/root/helix/work/workitem/baseservices/threading/Reports/baseservices.threading/coverage/OSThreadId/osthreadid/' is denied.
      Expected: True
      Actual:   False
      Stack Trace:
        /__w/1/s/bin/tests/Linux.arm.Checked/TestWrappers/baseservices.threading/baseservices.threading.XUnitWrapper.cs(89,0): at baseservices_threading._coverage_OSThreadId_osthreadid_osthreadid_._coverage_OSThreadId_osthreadid_osthreadid_sh()
    baseservices/threading/paramthreadstart/ThreadStartBool/ThreadStartBool.sh [FAIL]

@RussKeldorph is this known?

I think it is unlikely that the ARM coverage is specifically important here (vs x64, ARM64 etc). I am going to merge.

RussKeldorph · 2019-06-19T16:17:32Z

@MattGal @ilyas1974 It looks like the Helix workitem directory is not writable on some, but not all linux-arm agents. Are the new agents being added to the pool with correct configuration?

MattGal · 2019-06-19T16:39:51Z

@RussKeldorph can I see some of the failing / passing logs here? A single run demonstrating it would be great.

RussKeldorph · 2019-06-19T17:02:56Z

@MattGal is this enough: https://dev.azure.com/dnceng/public/_build/results?buildId=230439&view=logs&jobId=9c532748-9ca4-586a-2db0-c47bd86f8652?

RussKeldorph · 2019-06-19T17:03:26Z

https://mc.dot.net/#/user/coreclr-ci/pr~2Fdotnet~2Fcoreclr~2Frefs~2Fpull~2F25255~2Fmerge/test~2Ffunctional~2Fcli~2F/20190619.10

MattGal · 2019-06-19T17:04:57Z

@RussKeldorph yes exactly that. I'll see what I can find and get back.

MattGal · 2019-06-19T17:25:16Z

Filed https://github.com/dotnet/core-eng/issues/6763 to track and I'll take the first stab at investigting myself; current hypothesis is an assumption about the user id of the (outside-the-machine) owner of the file may be incorrect on some machines, leading to a different UID and permissions issues.

This test was checking for "Parameter name:" substring in the ArgumentNullException message. But a recent change dotnet#25185 modified that string to just "Parameter".

stephentoub · 2019-06-20T13:57:49Z

src/System.Private.CoreLib/shared/System/Exception.cs

    [System.Runtime.CompilerServices.TypeForwardedFrom("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")]
    public partial class Exception : ISerializable
    {
+        internal protected const string InnerExceptionPrefix = " ---> ";


@danmosemsft, did you mean for this to be private protected rather than internal protected? As it stands it's exposing this all derived types. It's thus also breaking the corefx build.

This test was checking for "Parameter name:" substring in the ArgumentNullException message. But a recent change dotnet#25185 modified that string to just "Parameter". We also have a coverage for this in corefx tests, so I am removing this test.

This test was checking for "Parameter name:" substring in the ArgumentNullException message. But a recent change #25185 modified that string to just "Parameter". We also have a coverage for this in corefx tests, so I am removing this test.

This test was checking for "Parameter name:" substring in the ArgumentNullException message. But a recent change dotnet#25185 modified that string to just "Parameter". We also have a coverage for this in corefx tests, so I am removing this test.

* Change ArgumentException message to one line * Disable 2 tests * Remove duplication in AggregateException.ToString() * Inner exceptions on new lines * Caps letter * Typo Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

* Change ArgumentException message to one line * Disable 2 tests * Remove duplication in AggregateException.ToString() * Inner exceptions on new lines * Caps letter * Typo Commit migrated from dotnet/coreclr@15ece8e

This test was checking for "Parameter name:" substring in the ArgumentNullException message. But a recent change dotnet/coreclr#25185 modified that string to just "Parameter". We also have a coverage for this in corefx tests, so I am removing this test. Commit migrated from dotnet/coreclr@b1ea5ed

danmoseley requested a review from jkotas June 15, 2019 20:14

danmoseley mentioned this pull request Jun 15, 2019

Show nested exceptions more clearly #25045

Closed

jkotas reviewed Jun 17, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/Exception.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 17, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/AggregateException.cs Show resolved Hide resolved

jkotas reviewed Jun 17, 2019

View reviewed changes

src/System.Private.CoreLib/Resources/Strings.resx Outdated Show resolved Hide resolved

jkotas reviewed Jun 17, 2019

View reviewed changes

tests/CoreFX/CoreFX.issues.rsp Show resolved Hide resolved

jkotas closed this Jun 17, 2019

jkotas reopened this Jun 17, 2019

danmoseley force-pushed the exformat30 branch from 7d13746 to 31ac63c Compare June 18, 2019 03:20

danmoseley added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 18, 2019

danmoseley added 6 commits June 18, 2019 15:21

Change ArgumentException message to one line

fcff40f

Disable 2 tests

4704850

Remove duplication in AggregateException.ToString()

c7bb252

Inner exceptions on new lines

dda0942

Caps letter

8d1b040

Typo

f68f4e7

danmoseley force-pushed the exformat30 branch from 90c60bb to f68f4e7 Compare June 18, 2019 22:24

danmoseley removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 19, 2019

danmoseley merged commit 15ece8e into dotnet:master Jun 19, 2019

danmoseley deleted the exformat30 branch June 19, 2019 08:28

janvorli added a commit to janvorli/coreclr that referenced this pull request Jun 20, 2019

Fix test 366085

1683565

This test was checking for "Parameter name:" substring in the ArgumentNullException message. But a recent change dotnet#25185 modified that string to just "Parameter".

janvorli mentioned this pull request Jun 20, 2019

Delete test 366085 #25277

Merged

stephentoub reviewed Jun 20, 2019

View reviewed changes

jkotas added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jun 23, 2019

BrennanConroy mentioned this pull request Jun 24, 2019

[master] Update dependencies from dotnet/core-setup dotnet/arcade dotnet/extensions#1873

Merged

JunTaoLuo mentioned this pull request Jun 25, 2019

[master] Update dependencies from 3 repositories dotnet/aspnetcore#11531

Merged

khellang mentioned this pull request Dec 21, 2021

Add netstandard2.0 target WalkerCodeRanger/semver#66

Closed

chiangvincent mentioned this pull request Feb 8, 2023

Target Based Scaling for CosmosDB + Separate out CosmosDBScaleMonitor from Listener Azure/azure-webjobs-sdk-extensions#806

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exception format cleanup subset#25185

Exception format cleanup subset#25185
danmoseley merged 6 commits intodotnet:masterfrom
danmoseley:exformat30

danmoseley commented Jun 15, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkotas commented Jun 17, 2019

Uh oh!

danmoseley commented Jun 18, 2019

Uh oh!

danmoseley commented Jun 19, 2019

Uh oh!

RussKeldorph commented Jun 19, 2019

Uh oh!

MattGal commented Jun 19, 2019

Uh oh!

RussKeldorph commented Jun 19, 2019

Uh oh!

RussKeldorph commented Jun 19, 2019

Uh oh!

MattGal commented Jun 19, 2019

Uh oh!

MattGal commented Jun 19, 2019

Uh oh!

stephentoub Jun 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

danmoseley commented Jun 15, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkotas commented Jun 17, 2019

Uh oh!

danmoseley commented Jun 18, 2019

Uh oh!

danmoseley commented Jun 19, 2019

Uh oh!

RussKeldorph commented Jun 19, 2019

Uh oh!

MattGal commented Jun 19, 2019

Uh oh!

RussKeldorph commented Jun 19, 2019

Uh oh!

RussKeldorph commented Jun 19, 2019

Uh oh!

MattGal commented Jun 19, 2019

Uh oh!

MattGal commented Jun 19, 2019

Uh oh!

stephentoub Jun 20, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants