[nnyeah] Centralize error handling #15064

chamons · 2022-05-19T15:58:04Z

Due to dotnet test crashes with System.Exception: Unable to read beyond the end of the stream if code under tests calls System.Environment.Exit microsoft/vstest#3658 it is not possible to test code that exits the process on error
Create a base class for nnyeah exceptions that we want to explicitly report (and not crash), ConversionException
Move Main to Main2 and wrap it in a try/catch for this exception
~~I did not catch everything as for now I want to crash with stack trace on unexpected failures~~

- Due to microsoft/vstest#3658 it is not possible to test code that exits the process on error - Create a base class for nnyeah exceptions that we want to explicitly report (and not crash), ConversionException - Move Main to Main2 and wrap it in a try/catch for this exception - I did not catch everything as for now I want to crash with stack trace on unexpected failures

rolfbjarne

One more reason Environment.Exit is evil 👿

tools/nnyeah/nnyeah/Program.cs

rolfbjarne · 2022-05-19T16:04:54Z

tools/nnyeah/nnyeah/Program.cs

+			catch (ConversionException c) {
+				Console.Error.WriteLine (c.Message);
+				return 1;
+			}


You'll want to catch all exceptions too, otherwise the process will exit with a very ugly message.

Alright, I wasn't sure here.

I'd add the following, do catch our base exception, then catch all exceptions and do a console.writeline with the message and then the url to provide an issue. If we got an unexpected exception it should be a bug/issue just like we do in xamarin.

rolfbjarne · 2022-05-19T16:06:16Z

tools/nnyeah/nnyeah/Program.cs

+			else {
+				ProcessAssembly (xamarinAssembly!, microsoftAssembly!, infile!, outfile!, verbose, forceOverwrite, suppressWarnings);
+			}


If you changed this method to return the exit code, you won't need this change, you can just return 0; after PrintOptions.

I had that before. I felt that showing the flow as explicit "if show help do that, else do all of the processing" was better than an early return.

Thoughts?

Yeah, it's more of a style choice, either way is fine with me.

Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>

rolfbjarne · 2022-05-19T16:14:30Z

tools/nnyeah/nnyeah/Program.cs

+			else {
+				ProcessAssembly (xamarinAssembly!, microsoftAssembly!, infile!, outfile!, verbose, forceOverwrite, suppressWarnings);
+			}


Yeah, it's more of a style choice, either way is fine with me.

vs-mobiletools-engineering-service2 · 2022-05-19T17:58:44Z

🔥 Tests failed catastrophically on VSTS: simulator tests iOS (no summary found). 🔥

Result file D:\a\1\s\Reports\TestSummary-simulator\TestSummary.md not found.

Pipeline on Agent XAMBOT-1042.Monterey
Merge 2597435 into 1477c9b

mandel-macaque · 2022-05-19T18:28:14Z

tools/nnyeah/nnyeah/Program.cs

+			catch (ConversionException c) {
+				Console.Error.WriteLine (c.Message);
+				return 1;
+			}


I'd add the following, do catch our base exception, then catch all exceptions and do a console.writeline with the message and then the url to provide an issue. If we got an unexpected exception it should be a bug/issue just like we do in xamarin.

chamons · 2022-05-20T16:56:10Z

I split uncaught exception and conversion exception, and print the full stack trace and a generic "file an issue" message for uncaught now:

Unexpected error - Please file a bug report at https://github.com/xamarin/xamarin-macios/issues/new
System.NotImplementedException: The method or operation is not implemented.
   at Microsoft.MaciOS.Nnyeah.Program.Main2(String[] args) in /Users/donblas/Programming/xamarin-macios/tools/nnyeah/nnyeah/Program.cs:line 62
   at Microsoft.MaciOS.Nnyeah.Program.Main(String[] args) in /Users/donblas/Programming/xamarin-macios/tools/nnyeah/nnyeah/Program.cs:line 24
donblas@Masterwork nnyeah % ~/bin/donut rung

vs

File for conversion (--input) and output location (--output) are required options.

vs-mobiletools-engineering-service2 · 2022-05-20T22:03:46Z

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

✅ All tests on macOS Mac Catalina (10.15) passed.

Pipeline on Agent
Hash: 0b89a17580fa2c08c28be477b7922f9e3d3ea0b9

vs-mobiletools-engineering-service2 · 2022-05-20T22:06:19Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMBOT-1160.Monterey'
Hash: 0b89a17580fa2c08c28be477b7922f9e3d3ea0b9

vs-mobiletools-engineering-service2 · 2022-05-20T22:12:37Z

🔥 Tests failed catastrophically on VSTS: simulator tests iOS (no summary found). 🔥

Result file D:\a\1\s\Reports\TestSummary-simulator\TestSummary.md not found.

Pipeline on Agent XAMBOT-1044.Monterey'
Merge 0b89a17 into b773b97

vs-mobiletools-engineering-service2 · 2022-05-20T22:21:23Z

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

Failed tests are:

xammac_tests
monotouch-test

Pipeline on Agent
Hash: 0b89a17580fa2c08c28be477b7922f9e3d3ea0b9

vs-mobiletools-engineering-service2 · 2022-05-20T22:29:05Z

📋 [PR Build] API Diff 📋

API diff (for current PR)

ℹ️ API Diff (from PR only) (please review changes)

API diff: vsdrops gist

Xamarin

iOS vsdrops gist
macOS vsdrops gist
tvOS vsdrops gist
watchOS vsdrops gist

.NET

dotnet-iOS vsdrops gist
dotnet-macOS vsdrops gist
dotnet-tvOS vsdrops gist
dotnet-MacCatalyst vsdrops gist

Xamarin vs .NET

dotnet-legacy-iOS vsdrops gist
dotnet-legacy-macOS vsdrops gist
dotnet-legacy-tvOS vsdrops gist

iOS vs Mac Catalyst (.NET)

dotnet-macCatiOS vsdrops gist

API diff (vs stable)

✅ API Diff from stable

API diff: vsdrops gist

Xamarin

iOS vsdrops gist
macOS vsdrops gist
tvOS vsdrops gist
watchOS vsdrops gist

.NET

dotnet-iOS vsdrops gist
dotnet-macOS vsdrops gist
dotnet-tvOS vsdrops gist
dotnet-MacCatalyst vsdrops gist

Xamarin vs .NET

dotnet-legacy-iOS vsdrops gist
dotnet-legacy-macOS vsdrops gist
dotnet-legacy-tvOS vsdrops gist

iOS vs Mac Catalyst (.NET)

dotnet-macCatiOS vsdrops gist

Generator diff

✅ Generator Diff (no change)

Pipeline on Agent XAMBOT-1100.Monterey'
Hash: 0b89a17580fa2c08c28be477b7922f9e3d3ea0b9

chamons requested review from dalexsoto, rolfbjarne, mandel-macaque and stephen-hawley May 19, 2022 15:58

chamons added skip-all-tests Skip all the tests not-notes-worthy Ignore for release notes labels May 19, 2022

Merge branch 'main' into nnyeah_error_refactor

bced388

rolfbjarne requested changes May 19, 2022

View reviewed changes

chamons and others added 2 commits May 19, 2022 11:09

Update tools/nnyeah/nnyeah/Program.cs

9f51ec9

Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>

Catch all exceptions

2597435

rolfbjarne approved these changes May 19, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

mandel-macaque approved these changes May 19, 2022

View reviewed changes

dalexsoto approved these changes May 20, 2022

View reviewed changes

Improve uncaught exception behavior

0b89a17

tj-devel709 approved these changes May 20, 2022

View reviewed changes

chamons merged commit 7e8bd5c into xamarin:main May 23, 2022

chamons deleted the nnyeah_error_refactor branch May 23, 2022 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[nnyeah] Centralize error handling #15064

[nnyeah] Centralize error handling #15064

chamons commented May 19, 2022 •

edited

Loading

rolfbjarne left a comment

rolfbjarne May 19, 2022

chamons May 19, 2022

chamons May 19, 2022

mandel-macaque May 19, 2022

rolfbjarne May 19, 2022

chamons May 19, 2022

rolfbjarne May 19, 2022

rolfbjarne May 19, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vs-mobiletools-engineering-service2 commented May 19, 2022

This comment has been minimized.

mandel-macaque May 19, 2022

chamons commented May 20, 2022

vs-mobiletools-engineering-service2 commented May 20, 2022

vs-mobiletools-engineering-service2 commented May 20, 2022

vs-mobiletools-engineering-service2 commented May 20, 2022

vs-mobiletools-engineering-service2 commented May 20, 2022

vs-mobiletools-engineering-service2 commented May 20, 2022

[nnyeah] Centralize error handling #15064

[nnyeah] Centralize error handling #15064

Conversation

chamons commented May 19, 2022 • edited Loading

rolfbjarne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vs-mobiletools-engineering-service2 commented May 19, 2022

🔥 Tests failed catastrophically on VSTS: simulator tests iOS (no summary found). 🔥

This comment has been minimized.

Choose a reason for hiding this comment

chamons commented May 20, 2022

vs-mobiletools-engineering-service2 commented May 20, 2022

💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻

vs-mobiletools-engineering-service2 commented May 20, 2022

📚 [PR Build] Artifacts 📚

Packages generated

vs-mobiletools-engineering-service2 commented May 20, 2022

🔥 Tests failed catastrophically on VSTS: simulator tests iOS (no summary found). 🔥

vs-mobiletools-engineering-service2 commented May 20, 2022

❌ [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) failed ❌

vs-mobiletools-engineering-service2 commented May 20, 2022

📋 [PR Build] API Diff 📋

API diff (for current PR)

API diff (vs stable)

Generator diff

chamons commented May 19, 2022 •

edited

Loading