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 tests #240

Merged
merged 3 commits into from Mar 12, 2018

Conversation

Projects
None yet
3 participants
@as-cii
Member

as-cii commented Mar 9, 2018

Description of the Change

In an attempt to make tests more reliable, I have decaffeinated spell-checks-spec.coffee, so that I could use async/await which, other than looking prettier, are hopefully more reliable than the equivalent CoffeeScript code.

Applicable Issues

#239

as-cii added some commits Mar 9, 2018

Temporarily disable tree-sitter parsers
Using tree-sitter parsers is unsupported at the moment because of the
changes to the grammar scope names (e.g., `source.js` -> `javascript).
@as-cii

This comment has been minimized.

Member

as-cii commented Mar 9, 2018

@lee-dohm: I have rebuilt this branch three times and tests seem to be much more reliable (I haven't observed any failure).

@maxbrunsfeld: can I talk you into taking a look at the TODO above? It seems like tree-sitter parsers are enabled by default on master and they are breaking spell-check because it still relies on legacy scope names to decide which grammars and syntactic constructs to spell-check.

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Mar 9, 2018

It seems like tree-sitter parsers are enabled by default on master

They're not enabled by default, but it looks like there's a bug that causes the tests to read your config.cson 😱 atom/atom#16910. I'll look into that asap.

@as-cii

This comment has been minimized.

Member

as-cii commented Mar 12, 2018

Thanks for jumping on that, @maxbrunsfeld. I'll go ahead and merge this pull request after reverting 30f0e9d. As for spell-check not working when tree-sitter is on, I am going to ignore that issue for the time being as I assume you were already aware of it.

@as-cii as-cii merged commit b5e8ff4 into master Mar 12, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@as-cii as-cii deleted the as-fix-tests branch Mar 12, 2018

@as-cii as-cii referenced this pull request Mar 12, 2018

Closed

Strange race condition in tests #239

1 of 1 task complete
@lee-dohm

This comment has been minimized.

Member

lee-dohm commented Mar 12, 2018

Thanks @maxbrunsfeld and @as-cii 🙇 This is great!

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Mar 12, 2018

As for spell-check not working when tree-sitter is on, I am going to ignore that issue for the time being as I assume you were already aware of it.

I think it was just a problem with the spell-check tests. They enabled spell-check for the source.js scope and assumed that opening a file ending in .js would use that scope, but it actually used the javascript scope.

@as-cii

This comment has been minimized.

Member

as-cii commented Mar 12, 2018

I think it was just a problem with the spell-check tests. They enabled spell-check for the source.js scope and assumed that opening a file ending in .js would use that scope, but it actually used the javascript scope.

Interesting. The main reason why I pointed that out was that after looking at the following lines of code:

  • spellCheckCurrentGrammar: ->
    grammar = @editor.getGrammar().scopeName
    _.contains(atom.config.get('spell-check.grammars'), grammar)
  • scopeIsExcluded: (scopeDescriptor, excludedScopes) ->
    @spellCheckModule.excludedScopeRegexLists.some (regexList) ->
    scopeDescriptor.scopes.some (scopeName) ->
    regexList.every (regex) ->
    regex.test(scopeName)

It seemed like the config settings could refer to the legacy scope names, which would not work when tree-sitter is on. What am I missing?

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Mar 13, 2018

Yeah, that's right. If someone actually wanted to spell-check JavaScript for some reason, right now they'd have to add javascript to their scopes setting rather than source.js; I haven't added logic to spell-check for dealing with the equivalent scopes, mostly because there's no Tree-sitter grammar for Markdown, Latex or other commonly-spellchecked languages. I thought you were saying there was some other sort of breakage in the package due to Tree-sitter.

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