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: update app spec to assert #13155

Merged
merged 7 commits into from Jun 14, 2018

Conversation

Projects
None yet
4 participants
@codebytere
Member

codebytere commented Jun 4, 2018

This PR begins a migration from our use of Node's assert testing module to expect with chai. This will both output more verbose and descriptive error messages as well as allow us to test things like promises as we begin to lay the groundwork for promisification of our APIs.

This PR may be divided into several (likely ~3) PRs to reduce the sheer number of changes made in any one PR.

@codebytere codebytere requested a review from electron/reviewers as a code owner Jun 4, 2018

@codebytere codebytere changed the title from [WIP] Migration of specs to use expect over assert to [WIP] spec: use expect over assert Jun 7, 2018

@codebytere

This comment has been minimized.

Show comment
Hide comment
@codebytere

codebytere Jun 7, 2018

Member

Will also remove spec/package-lock.json, that was not intentionally added

Member

codebytere commented Jun 7, 2018

Will also remove spec/package-lock.json, that was not intentionally added

@alexeykuzmin

This comment has been minimized.

Show comment
Hide comment
@alexeykuzmin

alexeykuzmin Jun 11, 2018

Contributor

@codebytere There are linter errors.

Contributor

alexeykuzmin commented Jun 11, 2018

@codebytere There are linter errors.

@jkleinsc

This comment has been minimized.

Show comment
Hide comment
@jkleinsc

jkleinsc Jun 11, 2018

Contributor

@codebytere what about breaking this into a PR per spec file? I know that means lots of PRs but it might be easier that way from both a review and coding perspective.

Contributor

jkleinsc commented Jun 11, 2018

@codebytere what about breaking this into a PR per spec file? I know that means lots of PRs but it might be easier that way from both a review and coding perspective.

@@ -238,7 +239,7 @@ describe('app module', () => {
let server = null
const socketPath = process.platform === 'win32' ? '\\\\.\\pipe\\electron-app-relaunch' : '/tmp/electron-app-relaunch'
beforeEach((done) => {
beforeEach(done => {

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jun 12, 2018

Contributor

Do we have it somewhere in the style guide?
That brackets should be omitted if there is only one argument.

@alexeykuzmin

alexeykuzmin Jun 12, 2018

Contributor

Do we have it somewhere in the style guide?
That brackets should be omitted if there is only one argument.

This comment has been minimized.

@codebytere

codebytere Jun 13, 2018

Member

in some cases yes, but since omitting brackets adds an implicit return i tend not to do that lest i introduce side effects

@codebytere

codebytere Jun 13, 2018

Member

in some cases yes, but since omitting brackets adds an implicit return i tend not to do that lest i introduce side effects

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jun 13, 2018

Contributor

I meant just done => instead of (done) =>.
There's no difference except in a code style.

@alexeykuzmin

alexeykuzmin Jun 13, 2018

Contributor

I meant just done => instead of (done) =>.
There's no difference except in a code style.

This comment has been minimized.

@codebytere

codebytere Jun 13, 2018

Member

on oh sorry, brackets typically mean [ and ] so i got confused :)

@codebytere

codebytere Jun 13, 2018

Member

on oh sorry, brackets typically mean [ and ] so i got confused :)

Show outdated Hide outdated spec/api-app-spec.js
Show outdated Hide outdated spec/api-app-spec.js
Show outdated Hide outdated spec/api-app-spec.js
Show outdated Hide outdated spec/api-app-spec.js
Show outdated Hide outdated spec/api-app-spec.js
Show outdated Hide outdated spec/api-app-spec.js
Show outdated Hide outdated spec/api-app-spec.js
Show outdated Hide outdated spec/api-app-spec.js
Show outdated Hide outdated spec/api-browser-view-spec.js
@codebytere

This comment has been minimized.

Show comment
Hide comment
@codebytere

codebytere Jun 13, 2018

Member

I'll break this into a series of PRs to simplify review!

Member

codebytere commented Jun 13, 2018

I'll break this into a series of PRs to simplify review!

codebytere added some commits Jun 13, 2018

@codebytere codebytere changed the title from [WIP] spec: use expect over assert to spec: app - use expect over assert Jun 13, 2018

@codebytere codebytere changed the title from spec: app - use expect over assert to spec: update app spec to assert Jun 13, 2018

codebytere added some commits Jun 13, 2018

@codebytere codebytere merged commit 9155919 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

@codebytere codebytere deleted the expect-app-spec branch Jun 14, 2018

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