-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
Update runtime metrics tests to avoid failures on some platforms #105300
Conversation
/azp run runtime-ioslikesimulator,runtime-maccatalyst |
Azure Pipelines successfully started running 2 pipeline(s). |
@matouskozak I'm taking a look at the latest failures. |
@matouskozak I'm hoping the next run is good! |
/azp run runtime-ioslikesimulator,runtime-maccatalyst |
Azure Pipelines successfully started running 2 pipeline(s). |
@stevejgordon thanks for helping with that. Is it possible to add the fix for #105203 in this PR too? |
I was hoping this would solve those too. |
/azp run runtime-androidemulator |
Azure Pipelines successfully started running 1 pipeline(s). |
@stevejgordon looks the tests still failing. Most likely we need to skip running the cpu time test on iOS and tvos I guess.
|
/azp run runtime-ioslikesimulator,runtime-maccatalyst,runtime-androidemulator |
Azure Pipelines successfully started running 3 pipeline(s). |
@stevejgordon looks we need more tweaks; it is failing in tvos.
|
@tarekgh, sorry for the delay. I've been knocked out with a virus. I'll hopefully be back to work next week and can tweak this then if that's not too late. |
4f89894
to
7109a74
Compare
@stevejgordon I have added a small change to the PR hopefully this will make the test pass. I have rebased with the main to ensure we have the latest from the main. |
/azp run runtime-ioslikesimulator,runtime-maccatalyst,runtime-androidemulator |
Azure Pipelines successfully started running 3 pipeline(s). |
/ba-g the failing test didn't run at all and not producing any logs. https://dev.azure.com/dnceng-public/public/_build/results?buildId=760893&view=ms.vss-test-web.build-test-results-tab&runId=19324756&resultId=100028&paneView=dotnet-dnceng.dnceng-build-release-tasks.helix-test-information-tab |
Was it intended to disable the tests on mobile by 7109a74 ? Based on the PR description, I thought the idea of this PR was to modify the tests so that they are able to run on mobile as well. From tvos-sim log:
|
Look at my comment #105300 (comment). The test was failing on the tvos too. I don't think we can claim these metrics are supported on such platforms without doing more work I guess. |
This updates the jit metrics assertions to only assertions for greater than zero measurements when the JIT appears to have run.
I've further limited the GcCollectionsCount test to allow zero measurements, which is best in cases where a forced GC may not have run.
This should fix #105202 and #105203.
What do you think of this approach @tarekgh? We could also more stringently skip platforms with an
IsNotMobile
check, but I think it's worth having these run if possible while allowing zero values for measurements when those occur. We mostly want to test that the metrics return a measurement for the expected metric name, and the value is less important and less deterministic.