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

Pass `uploadToServer` option to windows crash reporter #9053

Merged
merged 3 commits into from Mar 31, 2017

Conversation

Projects
None yet
2 participants
@tarruda
Contributor

tarruda commented Mar 29, 2017

Note that get/setUploadToServer is not implemented because the CustomInfo structure is only set when start is called, so to change the value of uploadToServer the user needs to call start again(which is how it already works for most options).

@tarruda tarruda requested a review from kevinsawicki Mar 29, 2017

@kevinsawicki kevinsawicki merged commit 8a01ebe into master Mar 31, 2017

8 of 9 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #6067440 succeeded in 72s
Details
electron-linux-ia32 Build #6067441 succeeded in 65s
Details
electron-linux-x64 Build #6067442 succeeded in 151s
Details
electron-mas-x64 Build #3778 succeeded in 8 min 23 sec
Details
electron-osx-x64 Build #3774 succeeded in 9 min 6 sec
Details
electron-win-ia32 Build #2770 succeeded in 8 min 25 sec
Details
electron-win-x64 Build #2741 succeeded in 8 min 24 sec
Details

@kevinsawicki kevinsawicki deleted the enable-crash-reporter-upload-windows branch Mar 31, 2017

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Mar 31, 2017

Contributor

Thanks for adding support for this @tarruda 👍 🏁

Contributor

kevinsawicki commented Mar 31, 2017

Thanks for adding support for this @tarruda 👍 🏁

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Apr 3, 2017

Contributor

Just got a flaky looking failure here on AppVeyor:

not ok 180 crashReporter module without sandbox should not send minidump if uploadToServer is false
  Error: EBUSY: resource busy or locked, unlink 'C:\Users\appveyor\AppData\Local\Temp\1\electronCrashReporterSpec-11733-1148-1priygi.7pgidaemi\Zombies Crashes\a92fbc58-7a87-4fd3-8012-3c70de2df434.dmp'
      at Object.fs.unlinkSync (fs.js:1007:18)
      at testDone (C:\projects\electron\spec\api-crash-reporter-spec.js:96:14)

https://ci.appveyor.com/project/electron-bot/electron/build/244/job/rp59bmb2epvvbh52#L1518

Perhaps we should use graceful-fs or ignore delete errors?

Contributor

kevinsawicki commented Apr 3, 2017

Just got a flaky looking failure here on AppVeyor:

not ok 180 crashReporter module without sandbox should not send minidump if uploadToServer is false
  Error: EBUSY: resource busy or locked, unlink 'C:\Users\appveyor\AppData\Local\Temp\1\electronCrashReporterSpec-11733-1148-1priygi.7pgidaemi\Zombies Crashes\a92fbc58-7a87-4fd3-8012-3c70de2df434.dmp'
      at Object.fs.unlinkSync (fs.js:1007:18)
      at testDone (C:\projects\electron\spec\api-crash-reporter-spec.js:96:14)

https://ci.appveyor.com/project/electron-bot/electron/build/244/job/rp59bmb2epvvbh52#L1518

Perhaps we should use graceful-fs or ignore delete errors?

@tarruda

This comment has been minimized.

Show comment
Hide comment
@tarruda

tarruda Apr 3, 2017

Contributor

Perhaps we should use graceful-fs or ignore delete errors?

The file is deleted is to ensure the crash dump is created(and not uploaded), my guess is that the crash reporter was still writing to the file when unlink was called. The reason I explicitly unlink the dump is because of some differences in how crashReporter directories are handled(it seems on mac they are initialized at startup and not changed later?), causing dumps from other tests to leak into this test.

In any case, I think it is better to not depend on unlink to know that the dump was created. I will send a PR to fix this soon.

Contributor

tarruda commented Apr 3, 2017

Perhaps we should use graceful-fs or ignore delete errors?

The file is deleted is to ensure the crash dump is created(and not uploaded), my guess is that the crash reporter was still writing to the file when unlink was called. The reason I explicitly unlink the dump is because of some differences in how crashReporter directories are handled(it seems on mac they are initialized at startup and not changed later?), causing dumps from other tests to leak into this test.

In any case, I think it is better to not depend on unlink to know that the dump was created. I will send a PR to fix this soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment