Skip to content
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

Json for Sentry config #80

Merged
merged 17 commits into from
Mar 30, 2021
Merged

Json for Sentry config #80

merged 17 commits into from
Mar 30, 2021

Conversation

semuserable
Copy link
Contributor

@semuserable semuserable commented Mar 19, 2021

No ScriptableObject for Sentry config. Quick & dirty.

  • points
    • UnitySentryOptions reused
    • link.xml must be placed inside Unity project for IL2CPP builds to work
  • tested on
    • Editor
    • Windows
      • IL2CPP
      • Mono
    • Android
      • API 30 (Android 11, ARM, please read notes below) via Android Studio emulator
        • IL2CPP - *.apk size ~19mb
        • Mono - *.apk size ~29mb

Note: currently the json config generated dynamically and not included in the project.

Android notes: Unity dropped support for x86_x86 for Android. Android Studio recommendation is to use emulator images which are based on x86, but Unity builds only for ARM. ARM is much slower in terms of performance. Google recently made an update for x86 images to support ARM in a performant way, but it works only for API 30 (Android 11).

If you try to use any previous ARM based images which are not tuned for x86, you will see the following message
image

It's so slow that I didn't wait enough for it to start...

This is an example of how it looks inside AVD. Android 11 uses x86-ARM-comptaible arch, while Android 4.4 uses plain-old-ARM

image

Another option is to use something like Blustacks or Genymotion (they also provide Android-as-a-Service). I had some Hyper-V problems running Bluestacks, because of a Docker which is always running on my system.

@semuserable
Copy link
Contributor Author

semuserable commented Mar 19, 2021

Please notice, currently the json config generated dynamically and not included in the project.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great already!
I left some notes to discuss and suggestions for the future

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach is great! Will also perform better and since we're anyway dependant on S.T.J might as well use it.

Good thing now we can take advantage of that nice testing infrastrucutre you've setup to get these code tested. Will be particularly nice to make sure the serialization round-trip works. Would also be good to have some snapshot tests to validate we're not introducing breakage on serialzied files, as we ship new versions

Comment on lines +64 to +73
public void SentryTestWindow_OpenAndLinkXmlCopied_Successful()
{
LogAssert.ignoreFailingMessages = true; // mandatory

// Open & Close window to trigger 'link.xml' logic
SentryTestWindow.Open().Dispose();

var linkXmlPath = $"{Application.dataPath}/Resources/{UnitySentryOptions.ConfigRootFolder}/link.xml";
Assert.IsTrue(File.Exists(linkXmlPath));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about we make sure it wasn't there to begin with. Or should we delete it before the test runs?

Suggested change
public void SentryTestWindow_OpenAndLinkXmlCopied_Successful()
{
LogAssert.ignoreFailingMessages = true; // mandatory
// Open & Close window to trigger 'link.xml' logic
SentryTestWindow.Open().Dispose();
var linkXmlPath = $"{Application.dataPath}/Resources/{UnitySentryOptions.ConfigRootFolder}/link.xml";
Assert.IsTrue(File.Exists(linkXmlPath));
}
public void SentryTestWindow_OpenAndLinkXmlCopied_Successful()
{
var linkXmlPath = $"{Application.dataPath}/Resources/{UnitySentryOptions.ConfigRootFolder}/link.xml";
Assert.IsFalse(File.Exists(linkXmlPath)); // sanity check
LogAssert.ignoreFailingMessages = true; // mandatory
// Open & Close window to trigger 'link.xml' logic
SentryTestWindow.Open().Dispose();
Assert.IsTrue(File.Exists(linkXmlPath));
}

var jsonRaw = File.ReadAllText(optionsFilePath);
using var jsonDocument = JsonDocument.Parse(jsonRaw);

UnitySentryOptions.FromJson(jsonDocument.RootElement);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing the asserts I assume?

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good to be merged though, the other notes could be a follow up PR.

Thanks!

@bruno-garcia bruno-garcia merged commit 9d82923 into main Mar 30, 2021
@bruno-garcia bruno-garcia deleted the exp/json-config branch March 30, 2021 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants