Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add System.Diagnostics.StackTrace to test runtime dependencies#12567

Merged
tmat merged 1 commit intodotnet:masterfrom
tmat:AddStackTrace
Oct 13, 2016
Merged

Add System.Diagnostics.StackTrace to test runtime dependencies#12567
tmat merged 1 commit intodotnet:masterfrom
tmat:AddStackTrace

Conversation

@tmat
Copy link
Copy Markdown
Member

@tmat tmat commented Oct 11, 2016

The library was missing from test output directory, which prevented xunit to display stack traces with source locations.

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Oct 11, 2016

@MattGal

@stephentoub
Copy link
Copy Markdown
Member

This fixes the lack-of-line-numbers-in-exception-stack-traces problem? If so, excellent!

@MattGal
Copy link
Copy Markdown
Member

MattGal commented Oct 11, 2016

LGTM. I would only want to consult with @ericstj and make sure that including this by default with all tests is not going to be an issue (i.e. it may need to be specificallly included only in certain groups)

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Oct 11, 2016

@stephentoub Yes. Seems like it should.

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Oct 12, 2016

test Innerloop Ubuntu14.04 Debug Build and Test please

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Oct 12, 2016

test Innerloop Ubuntu14.04 Release Build and Test please

@MattGal
Copy link
Copy Markdown
Member

MattGal commented Oct 12, 2016

@tmat , with the dependency-versioning issue fixed, it's clear that there is a serious effect (at least to logging?) of including this DLL on Ubuntu runs. We'll need to figure that out (may be a product issue) before you can merge, as this will affect all tests running on Ubuntu all the time.

*Update: This problem is only in logging and thus may not be a major issue. It definitely seems to make the CI logs very, very long for certain tests.

@ericstj
Copy link
Copy Markdown
Member

ericstj commented Oct 12, 2016

It looks like the growth in log is only for Ubuntu debug. I believe this was introduced with #12287. /cc @jamesqo

LGTM.

@stephentoub
Copy link
Copy Markdown
Member

It looks like the growth in log is only for Ubuntu debug. I believe this was introduced with #12287. /cc @jamesqo

We need to either remove the assert that's getting triggered, or remove the tests that are hitting the assert. I'd prefer the former: if it's possible to hit an assert from a test, assuming it's via publicly exposed surface area, then either there's a bug in the implementation or the assert is invalid.

@ericstj
Copy link
Copy Markdown
Member

ericstj commented Oct 12, 2016

Agreed, I pinged @jamesqo in his PR that added these.

@jamesqo
Copy link
Copy Markdown
Contributor

jamesqo commented Oct 12, 2016

Sorry for this. Fixed the issue in #12599 by removing the relevant tests

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Oct 12, 2016

OK, I'll wait for #12599 before merging this.

@stephentoub
Copy link
Copy Markdown
Member

OK, I'll wait for #12599 before merging this.

It's been merged. Though due to other things having been merged you're unfortunately going to need to rebase to address a merge conflict.

@tmat tmat merged commit f4b143b into dotnet:master Oct 13, 2016
@tmat tmat deleted the AddStackTrace branch October 13, 2016 00:43
@karelz karelz modified the milestone: 1.2.0 Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
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.

7 participants