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

Add linter to spec/ folder #18896

Merged
merged 8 commits into from Feb 28, 2019

Conversation

Projects
None yet
3 participants
@rafeca
Copy link
Contributor

commented Feb 22, 2019

Identify the Bug

Linter was not checking the spec/ folder.

Description of the Change

Note: This PR depends on #18885

The JS code style of the spec/ folder is not matching fully the style of the rest of the repository, this is caused by not having the linter running on this folder.

This PR enables the linter and fixes all the linting issues that appeared on that folder. Most of the issues have been fixed automatically by running prettier-standard on that folder, the things that needed to be fixed manually are in separate commits.

Alternate Designs

N/A

Possible Drawbacks

N/A (since this PR only changes test files).

Verification Process

Check that tests keep passing on CI.

Release Notes

N/A (it's only an internal change).

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Oh, since GitHub does not support stacked PRs, this PR is showing also the commits from #18885 . Please ignore the commit with message Enable linter on packages/ folder and all commits older than that one.

Edit: #18885 has been merged so the displayed commits in this PR are correct now.

@rafeca rafeca force-pushed the rafeca:add-linting-to-specs branch from fc16b81 to 2cbebe2 Feb 22, 2019

@jasonrudolph

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Oh, since GitHub does not support stacked PRs, this PR is showing also the commits from #18885 . Please ignore the commit with message Enable linter on packages/ folder and all commits older than that one.

@rafeca: Once #18885 is merged, would you mind re-requesting review on this PR so that people can easily review the changes that are specific to this PR?

@rafeca rafeca force-pushed the rafeca:add-linting-to-specs branch 3 times, most recently from 46c485d to a4bf3e2 Feb 25, 2019

@jasonrudolph
Copy link
Member

left a comment

Nice work! ⚡️

Most of the issues have been fixed automatically by running prettier-standard on that folder, the things that needed to be fixed manually are in separate commits.

I did not review the automatic fixes, but I did review each of the commits that performed manual fixes. I noted one question inline below, but overall, these changes look good. 👍

component.pixelPositionForScreenPosition(Point(11, Infinity))
})

await component.getNextUpdatePromise()

This comment has been minimized.

Copy link
@jasonrudolph

jasonrudolph Feb 26, 2019

Member

Does the linter mandate that we remove this block?

This use of blocks is a pattern we employ to exercise multiple scenarios in a single test, without having to come up with unique variable names for each scenario.

This comment has been minimized.

Copy link
@rafeca

rafeca Feb 26, 2019

Author Contributor

Yes, this is mandated by the no-lone-blocks rule, it only complains when there are no variables defined inside the block.

If this is a common pattern across the codebase I can disable this rule for the spec/ folders (I have to figure out how to do this since we're using the standard rules for linting).

This comment has been minimized.

Copy link
@jasonrudolph

jasonrudolph Feb 26, 2019

Member

If this is a common pattern across the codebase I can disable this rule for the spec/ folders (I have to figure out how to do this since we're using the standard rules for linting).

That works for me.

Alternatively, could we use an eslint-disable comment to tell the linter to ignore that particular rule in this particular spec?

rafeca added a commit to rafeca/atom that referenced this pull request Feb 26, 2019

Override global jasmine spec functions
Currently, if a spec uses the global `it` function on an async test,
that test will always pass (since the jasmine version checked in Atom
does not natively support tests that return promises). This can be
confusing since the test behaviour is different between the
async-test-helpers methods and the global ones.

By overriding the global functions, we'll also be able to remove all the
imports from async-test-helpers since they won't be needed anymore.

More info: atom#18896 (comment)

rafeca added a commit to rafeca/atom that referenced this pull request Feb 27, 2019

Override global jasmine spec functions
Currently, if a spec uses the global `it` function on an async test,
that test will always pass (since the jasmine version checked in Atom
does not natively support tests that return promises). This can be
confusing since the test behaviour is different between the
async-test-helpers methods and the global ones.

By overriding the global functions, we'll also be able to remove all the
imports from async-test-helpers since they won't be needed anymore.

More info: atom#18896 (comment)

rafeca added a commit to rafeca/atom that referenced this pull request Feb 27, 2019

Override global jasmine spec functions
Currently, if a spec uses the global `it` function on an async test,
that test will always pass (since the jasmine version checked in Atom
does not natively support tests that return promises). This can be
confusing since the test behaviour is different between the
async-test-helpers methods and the global ones.

By overriding the global functions, we'll also be able to remove all the
imports from async-test-helpers since they won't be needed anymore.

More info: atom#18896 (comment)

rafeca added a commit to rafeca/atom that referenced this pull request Feb 27, 2019

Override global jasmine spec functions
Currently, if a spec uses the global `it` function on an async test,
that test will always pass (since the jasmine version checked in Atom
does not natively support tests that return promises). This can be
confusing since the test behaviour is different between the
async-test-helpers methods and the global ones.

By overriding the global functions, we'll also be able to remove all the
imports from async-test-helpers since they won't be needed anymore.

More info: atom#18896 (comment)

@rafeca rafeca force-pushed the rafeca:add-linting-to-specs branch from c4e7696 to 3c80054 Feb 27, 2019

rafeca added a commit to rafeca/atom that referenced this pull request Feb 27, 2019

Override global jasmine spec functions
Currently, if a spec uses the global `it` function on an async test,
that test will always pass (since the jasmine version checked in Atom
does not natively support tests that return promises). This can be
confusing since the test behaviour is different between the
async-test-helpers methods and the global ones.

By overriding the global functions, we'll also be able to remove all the
imports from async-test-helpers since they won't be needed anymore.

More info: atom#18896 (comment)

rafeca added a commit to rafeca/atom that referenced this pull request Feb 27, 2019

Override global jasmine spec functions
Currently, if a spec uses the global `it` function on an async test,
that test will always pass (since the jasmine version checked in Atom
does not natively support tests that return promises). This can be
confusing since the test behaviour is different between the
async-test-helpers methods and the global ones.

By overriding the global functions, we'll also be able to remove all the
imports from async-test-helpers since they won't be needed anymore.

More info: atom#18896 (comment)

rafeca added a commit to rafeca/atom that referenced this pull request Feb 28, 2019

Override global jasmine spec functions
Currently, if a spec uses the global `it` function on an async test,
that test will always pass (since the jasmine version checked in Atom
does not natively support tests that return promises). This can be
confusing since the test behaviour is different between the
async-test-helpers methods and the global ones.

By overriding the global functions, we'll also be able to remove all the
imports from async-test-helpers since they won't be needed anymore.

More info: atom#18896 (comment)
@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

Once the CI tests pass I'm gonna merge #18917, which should address the issues that @Arcanemagus pointed out with it, fit, etc.

Would you folks be alright with merging this PR afterwards?

rafeca added a commit to rafeca/atom that referenced this pull request Feb 28, 2019

Override global jasmine spec functions
Currently, if a spec uses the global `it` function on an async test,
that test will always pass (since the jasmine version checked in Atom
does not natively support tests that return promises). This can be
confusing since the test behaviour is different between the
async-test-helpers methods and the global ones.

By overriding the global functions, we'll also be able to remove all the
imports from async-test-helpers since they won't be needed anymore.

More info: atom#18896 (comment)

rafeca added a commit to rafeca/atom that referenced this pull request Feb 28, 2019

Override global jasmine spec functions
Currently, if a spec uses the global `it` function on an async test,
that test will always pass (since the jasmine version checked in Atom
does not natively support tests that return promises). This can be
confusing since the test behaviour is different between the
async-test-helpers methods and the global ones.

By overriding the global functions, we'll also be able to remove all the
imports from async-test-helpers since they won't be needed anymore.

More info: atom#18896 (comment)
@Arcanemagus

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

Actually with #18917 merged almost all the imports of async-spec-helpers should be able to be removed entirely since the base versions now support async/await. It looks like there are a few cases where it would still be needed though (example).

That can be saved for another PR though as the same can likely be said for virtually all the specs.

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

Actually with #18917 merged almost all the imports of async-spec-helpers should be able to be removed entirely since the base versions now support async/await. It looks like there are a few cases where it would still be needed though (example).

Yup, I'm planning to remove all these imports in a follow-up PR 😄

@rafeca rafeca force-pushed the rafeca:add-linting-to-specs branch from 3c80054 to 2dd2c29 Feb 28, 2019

@rafeca rafeca merged commit aad8bd6 into atom:master Feb 28, 2019

3 checks passed

Atom Pull Requests #20190228.12 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rafeca rafeca deleted the rafeca:add-linting-to-specs branch Feb 28, 2019

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.