Conversation
|
@mikem8361 how do I debug a single test on linux/mac? The GCTests SOS test is failing but only on non-windows. |
|
It isn’t that easy. You can cut/paste all the lldb commands/paths from the log (something like ~/diagnostics/artifacts/TestResults/Debug/sos.unittests_2020_06_15_11_34_05_8509/SOS.GCTests.projectk.sdk.prebuilt.5.0.0-preview.7.20313.3.log) into a console and try to get the point the test fails.
|
| class GCHeapDetails | ||
| { | ||
| private: | ||
| void GetGenerationTableSize(CLRDATA_ADDRESS svrHeapAddr, unsigned int *count) |
There was a problem hiding this comment.
GetGenerationTableSize [](start = 9, length = 22)
nit - what was the reason to call this GetGenerationTableSize instead of GetNumberGenerations 'cause it seems like that's exactly what it does?
There was a problem hiding this comment.
It's a helper method in GCHeapDetails and not a public API, and the only place it's called is to get the size to allocate for the generation table. It could be named either way
| // | ||
| // | ||
| // |
There was a problem hiding this comment.
These whitespace only changes make it very difficult to review this change. Is there any way you could separate just the code changes into one PR and the whitespace only fixes into another? Or are they sufficiently merged together at this point? Chrome is having a real tough time rendering these large diffs.
There was a problem hiding this comment.
I tried for about a half hour today to undo the whitespace changes and can't without manually redoing everything. There apparently are lots of ways to remove trailing whitespace with git commands, but no way to preserve it I could find.
If you don't know - recently github added ignore whitespace to reviews, but it's kind of hidden. The settings button (gear icon at the top) should have a checkbox to ignore whitespace.
Sorry about the noise
…BERGENERATIONS + 3) but the runtime only returns 6 items.
…running against aruntime without a POH
noahfalk
left a comment
noahfalk
left a comment
There was a problem hiding this comment.
The SOS changes looked good to me! Are there some updated tests as well?
I have been manually testing and want to add an automated test but it's taking a little bit longer than expected |
|
@mikem8361 How do I coordinate adding a test for this? The SOS tests run against the preview bits so it won't pass until my runtime changes make it in to the preview builds |
|
You can develop the SOS tests in the diagnostics repo and use the Once you get the tests running, merge the runtime changes and wait up DARC pushes the new version to the diagnostics repo (one or days normally). Once your runtime version makes it into the diagnostics repo, you merge the new tests. I'd find some existing test script to add the new commands/reqex matching to like OtherCommands.script. It can be a real pain to get the regex right. Hopefully the existing script regexs will help. |
|
I am opening #1264 to add the test for this so I don't have to block merging this change. I have confirmed it passes with a private build. |
5 participants
Fixes #1068
Corresponding runtime changes in dotnet/runtime#37853. I have an editor extension that removes trailing whitespace, sorry for the large amount of whitespace changes. I was not aware how much trailing whitespace there was in the SOS sources.