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

spec: better texts for the Crash Reporter tests #13227

Merged
merged 2 commits into from Jun 14, 2018

Conversation

Projects
None yet
2 participants
@alexeykuzmin
Copy link
Contributor

alexeykuzmin commented Jun 12, 2018

No description provided.

@alexeykuzmin alexeykuzmin requested a review from MarshallOfSound Jun 12, 2018

@alexeykuzmin alexeykuzmin requested a review from as a code owner Jun 12, 2018


assert(reports.length > 1, 'has more than 1 report')

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jun 12, 2018

Author Contributor

'has more than 1 report' text was misleading.
There should be an error message instead of an expectation's description.

So instead of having "failed: has more than 1 report" as a failure message, it should read something like "failed: there are not enough reports".

const timestamp = new Date(cur.date).getTime()
return (timestamp > acc.timestamp)
? { report: cur, timestamp: timestamp }
: acc
}, { timestamp: 0 })
}, { timestamp: -Infinity })
assert(newestReport, 'Hey!')

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jun 12, 2018

Author Contributor

Not a part of the test. Is here only to check if we test the thing right.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Jun 14, 2018

Member

expect(newestReport).to.be.ok?

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jun 14, 2018

Author Contributor

It's not related to the test itself.
I like to use assertss for things like this one to explicitly separate them.

const timestamp = new Date(cur.date).getTime()
return (timestamp > acc.timestamp)
? { report: cur, timestamp: timestamp }
: acc
}, { timestamp: 0 })
}, { timestamp: -Infinity })

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jun 12, 2018

Author Contributor

const timestamp = new Date(cur.date).getTime()

getTime() returns an amount of seconds since the Epoch, and can be negative.
The test should work even if a test machine lives in the '60s.

@@ -256,23 +257,31 @@ describe('crashReporter module', () => {
})
})

// TODO(alexeykuzmin): This suite should explicitly
// generate several crash reports instead of hoping
// that there will be enough of them already.

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jun 12, 2018

Author Contributor

That's actually quite sad =/

@codebytere codebytere changed the title Better texts for the Crash Reporter tests spec: better texts for the Crash Reporter tests Jun 13, 2018

@MarshallOfSound
Copy link
Member

MarshallOfSound left a comment

left a nit re expect vs assert

@MarshallOfSound MarshallOfSound merged commit 6a59b37 into master Jun 14, 2018

11 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@MarshallOfSound MarshallOfSound deleted the better-texts-for-the-crash-reporter-tests branch Jun 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.