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

Added test framework and a couple of tests for for syntax highlighting #742

Merged
merged 4 commits into from
Sep 23, 2016

Conversation

ivanz
Copy link
Contributor

@ivanz ivanz commented Sep 2, 2016

Closes #696

The idea here was to wrap vscode-textmate in an easy to consume interface and expose a quick way to check if certain token was picked up correctly at a certain line and column number in a given input.

Had to update travis.yml to handle node native dependencies (vscode-textmate pulls in a native regex lib) and also added chai.js and its definitions.

Example

A minimal example looks like this:

const input = `
namespace TestNamespace {
}`;

let tokens: Token[] = TokenizerUtil.tokenize(input);
tokens.should.contain(Tokens.NamespaceKeyword("namespace", /*line*/ 2, /*column*/ 1));
tokens.should.contain(Tokens.NamespaceIdentifier("TestNamespace", 2, 11));

To add a new token type tokenizer.ts needs to be updated with the token details (this avoids hardcoding token id strings across tests and doing string operations):

export namespace Tokens {

   // <..snip...>

    export const NamespaceIdentifier = (text: string, line?: number, column?: number) =>
        createToken(text, "entity.name.type.namespace.cs", line, column);
}

Unrelated note: It would be great to re-visit the typings story when TypeScript 2.0 comes out with the @typings/* npm support, because having to manually version typings and store them in a directory is less than ideal in the long run. Given TS 2.0 is round the corner there is also probably no point investing time in using something like the typings or tsd tools. Just my 2c :)

@DustinCampbell
Copy link
Member

This is very cool @ivanz! That said, I'm not thrilled that "npm install" will require several prerequisites to run to completion with this change. Could you document these?

FWIW, we should consider the existing Roslyn colorizer tests here: https://github.com/dotnet/roslyn/tree/master/src/EditorFeatures/CSharpTest/Classification. I'm not sure how many we could get working properly with regex-based colorizer, but there's certainly some good tests to start from.

@DustinCampbell
Copy link
Member

I had to do the following to get npm install to work on my Windows machine:

  1. Run npm install --global --production windows-build-tools
  2. Modify my installation of VS 2015 to include the Windows 8.1 SDK
  3. Update npm with npm -g install npm@next

After that, I could successfully do the following:

npm install
npm run compile
npm test

> npm test

> [email protected] test C:\Projects\OmniSharp-VSCode
> mocha --timeout 15000 -u tdd ./out/test/**/*.tests.js



  Sanity Tests
    √ Boolean checks

  Grammar
    Class
      √ has a class keyword, a name and optional storage modifiers (79ms)

  Grammar
    Namespace
      √ has a namespace keyword and a name
      √ can be nested
      √ can contain using statements


  5 passing (108ms)

@ivanz
Copy link
Contributor Author

ivanz commented Sep 7, 2016

Thanks for haivng a look at this. I forgot that those steps are needed, because I already had the VS C++ compiler and Python installed on my machine (I can't recall having ever had to install the windows build tools package that you mention). 😞

It's not great to have to go through those extra hoops on Windows indeed. Maybe we can add the windows-build-tools package as a peerDependency given that it's a machine-global thing and as you say add something to the readme or a new Building.md or similar doc. I wonder if a Dockerfile for building and running tests on Linux will be of any help for those that don't want to install all the deps required by windows-build-tools...

I am on holiday and will be back in ~2 weeks when I will look at this again.

@ivanz
Copy link
Contributor Author

ivanz commented Sep 23, 2016

Hi,

I am back from holiday. I should probably make the windows node build tools package a peerDependency, because it's a windows-only global thing anyway. This and adding some docs about it is about the only thing I can do to make this less painful. What are you thoughts? (The only other thing I can think of is pulling out the grammar into its own npm package...)

At this point I am not willing personally to invest time in trying to port the Roslyn semantic-highlighting tests to TypeScript. The way I was thinking about this is that a good short-term win can be adding some tests for the most common regressions (things to do with method signatures seem to be one for example) rather than try to build out a full-blown test suit. Given that the grammar is becoming more and more complex (especially more so with the upcoming C# 7 features) it will be worth probably adding some tests for those in the future.

It's unfortunate that we don't even have a finger in the air estimation of when they will add the semantic highlighting api (microsoft/vscode#585 ) to know how much effort to put into this.

@DustinCampbell
Copy link
Member

All of that makes great sense to me @ivanz. I agree that moving the Roslyn semantic-highlighting tests to TypeScript would be wasted effort. I just wanted to point that there's a wealth of expectations about how C# expects to colorize code in those tests, and the syntax-based tests are separate from the semantic-based tests.

@ivanz ivanz force-pushed the ivan-syntax-highlighting-tests branch from 559c606 to db0b2a3 Compare September 23, 2016 20:35
@ivanz
Copy link
Contributor Author

ivanz commented Sep 23, 2016

Rebased and added info in README

@DustinCampbell DustinCampbell added this to the 1.5 milestone Sep 23, 2016
Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking pretty good. Did you want to add the peerDependency?

@@ -32,7 +32,6 @@ export default class OmniSharpCodeLensProvider extends AbstractSupport implement
};

provideCodeLenses(document: TextDocument, token: CancellationToken): CodeLens[] | Thenable<CodeLens[]> {
let request = { Filename: document.fileName };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄

@ivanz
Copy link
Contributor Author

ivanz commented Sep 23, 2016

Sorry - forgot to say that I did not end up adding it, because I thought that non-Windows devs will start getting a warning about a Windows-specific thing. Thoughts?

@DustinCampbell
Copy link
Member

Yeah, I agree.

@DustinCampbell
Copy link
Member

Let's do it.

@DustinCampbell DustinCampbell merged commit 2be4ef1 into dotnet:master Sep 23, 2016
@ivanz ivanz deleted the ivan-syntax-highlighting-tests branch December 6, 2016 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants