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

fix: Ensure that getLastCrashReport() is actually the last crash report #12253

Merged
merged 6 commits into from
Mar 14, 2018

Conversation

felixrieseberg
Copy link
Member

This PR fixes #11749 by ensuring that getLastCrashReport() actually returns the last crash report (and not just a certain position in the array).

Also: A test that actually calls the method under test 😅

@felixrieseberg felixrieseberg requested a review from a team March 13, 2018 23:58
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

LGTM 💯 when we merge this we can close the other PR

ckerr
ckerr previously requested changes Mar 14, 2018
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

This PR is short and sweet. LGTM. One nit inline.

assert(lastReport != null)
assert(lastReport === reports[0])
Copy link
Member

@ckerr ckerr Mar 14, 2018

Choose a reason for hiding this comment

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

It's not clear from this block that the new code is being tested.

Would it be possible to prepend an assert(reports.length > 1) to show that comparison happened?

@ckerr
Copy link
Member

ckerr commented Mar 14, 2018

electron-linux-x64:

AssertionError [ERR_ASSERTION]: false == true
    at Context.it (/home/builduser/project/spec/api-crash-reporter-spec.js:266:7)

electron-linux-ia32:

not ok 987 crashReporter module getLastCrashReport correctly returns the most recent report
  AssertionError [ERR_ASSERTION]: false == true
      at Context.it (/var/lib/jenkins/workspace/ron_ARM64_electron_PR-12253-WP3JY47LEEJ2OR6WRBG4W43ZPAXD4YTS3ZGJCPY3GXIOPM3Q5EKQ@2/spec/api-crash-reporter-spec.js:266:7)
      at runCallback (timers.js:763:18)
      at tryOnImmediate (timers.js:734:5)
      at processImmediate (timers.js:716:5)

@trop
Copy link
Contributor

trop bot commented Mar 14, 2018

An error occurred while attempting to backport this PR to "1-8-x", you will need to perform this backport manually

@trop
Copy link
Contributor

trop bot commented Mar 14, 2018

We have automatically backported this PR to "2-0-x", please check out #12255

@trop trop bot added merged/2-0-x and removed target/2-0-x labels Mar 14, 2018
@MarshallOfSound
Copy link
Member

@felixrieseberg Could you backport this to 1-8-x 😄 The bot did 2-0-x for you 👍

@ckerr
Copy link
Member

ckerr commented Mar 14, 2018

@MarshallOfSound IMO assert(reports.length > 1) was still warranted and we're literally in the same TZ so 👎 on the dismissal.

Not a blocker though so ¯\(ツ)/¯ on this

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.

CrashReporter.getLastCrashReport() does not return latest
4 participants