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

Update description with crash #627

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Conversation

urielsalis
Copy link
Member

Purpose

Fixes #281

Approach

image

Future work

Checklist

  • Included tests
  • Updated documentation in README and wiki
  • Tested in MCTEST-xxx

@Marcono1234 Marcono1234 mentioned this pull request May 11, 2021
2 tasks
Copy link
Contributor

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

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

Looks good!
But I think the intention with #281 was partially also to have the crash report easily visible in the description. Having Arisa add a comment might be useful as well, but it might be a different use case. @misode what do you think?

It might also become spammy when users upload a bunch of crash reports, e.g. "I am not sure which crash report you want so I uploaded all..."
But we can see if that actually becomes a problem.

@urielsalis
Copy link
Member Author

Looks good!
But I think the intention with #281 was partially also to have the crash report easily visible in the description. Having Arisa add a comment might be useful as well, but it might be a different use case. @misode what do you think?

It might also become spammy when users upload a bunch of crash reports, e.g. "I am not sure which crash report you want so I uploaded all..."
But we can see if that actually becomes a problem.

Main idea from what I understood is being able to search, as jira doesnt search attachments. Comments are less spammy than description too 👀

@misode
Copy link

misode commented May 11, 2021

In my opinion having the crash report at the bottom of the description would be better. Comments get pushed away most of the time. But now that I say that, editing descriptions also sounds risky, for example the reporter might be in the middle of editing while the module is triggered. (Although apparently the later user edit would overwrite the crash log, which means this wouldn't be a huge issue)

A question I have: what's the point of having a deobfuscated stack trace?

@Marcono1234
Copy link
Contributor

A question I have: what's the point of having a deobfuscated stack trace?

See #606, mainly:

  • to make it easier to reason about where an exception occurred
  • to make it easier to compare crash reports accross versions (when obfuscation mappings and line numbers change)

@misode
Copy link

misode commented May 12, 2021

Oh I'm tired. I meant what's the reason for the obfuscated stack trace in the comment? The deobfuscated stack trace should be easier to compare and give you all the information.

@urielsalis
Copy link
Member Author

urielsalis commented May 12, 2021 via email

Copy link
Member

@violine1101 violine1101 left a comment

Choose a reason for hiding this comment

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

Looks good tech-wise, but I have similar concerns as Misode and Marcono.

I don't think this should be left as a comment. As said in #281 already, I really only think this crash should be integrated into the description when the bug report is initially created.

This is most important if the crash report has been added to the description and the newlines have been messed up. In all other cases it's trivial for a mod or helper to copy stuff from attachments to the description, although it would still be a nice QoL change to not have to open an attachment when checking out a new crash bug report.

Adding a comment every time a crash report is added just causes spam, especially if someone is intentionally uploading unrelated crash reports to a ticket.

urielsalis and others added 4 commits May 13, 2021 09:20
@urielsalis urielsalis changed the title Add comment with crash [NEEDS TAKEOVER] Add comment with crash May 13, 2021
@chandler05 chandler05 changed the title [NEEDS TAKEOVER] Add comment with crash Add comment with crash May 21, 2021
@chandler05 chandler05 self-assigned this May 21, 2021
@chandler05 chandler05 changed the title Add comment with crash Update description with crash May 21, 2021
@@ -344,6 +344,26 @@ fun addRestrictedComment(
}
}

fun addRawComment(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed anymore, is it?

Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to leave it for future purposes.

Comment on lines +26 to +28
newCrashes
.filter { it.second is Crash.Minecraft }
.filterNot { it.first.name.endsWith("deobfuscated.txt") }
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably restrict this to crash reports attached by the reporter (or by a helper / moderator?).
Otherwise users can vandalize existing issues by attaching unrelated crash reports, or files which Arisa considers to be crash reports.

.filter { it.second is Crash.Minecraft }
.filterNot { it.first.name.endsWith("deobfuscated.txt") }
.forEach {
if (!description!!.contains("""\{code.*${it.first.name}]}(\S|\s)*\{code}""".toRegex())) {
Copy link
Contributor

@Marcono1234 Marcono1234 May 28, 2021

Choose a reason for hiding this comment

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

Is (\S|\s) to match any character including line breaks? Maybe the complete (\S|\s)*\{code} part can be removed, it is rather unlikely that the description contains {code:title=...} which is not actually a code block.

Additionally we should probably only add the crash report to the description if it does not contain any (instead of checking for the specific crash report name), to prevent adding multiple crash report snippets. Therefore we should probably only consider newCrashes if its size is 1, otherwise Arisa might pick the 'wrong' crash report in case a user uploads unrelated ones.

The regex could probably changed to this:

Suggested change
if (!description!!.contains("""\{code.*${it.first.name}]}(\S|\s)*\{code}""".toRegex())) {
// Check if description already contains code block which might contain crash report
// Note: Jira seems to allow spaces between most of the characters
if (!description!!.contains("""\{code:\s*title\s*=.*\[\^.+\.txt\s*]\s*}""".toRegex())) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this regex is too unspecific though and causes some false negatives, e.g. when {code} blocks link to a non-crash attachment (e.g. code snippet file when a code analysis was done). But I assume that this will happen rarely, so it might be fine for now and we can adjust it later if necessary.

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.

Copy crash report to the description when it is attached
5 participants