Skip to content

Initial smoke for usage of jest #17

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

Merged
merged 8 commits into from
Jun 1, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .babelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"presets": [
["@babel/preset-env", { "targets": { "node": "current" } } ],
"@babel/preset-typescript",
]
}
File renamed without changes.
20 changes: 20 additions & 0 deletions jest.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
module.exports = {
verbose: true,
projects: [
'<rootDir>'
],
testMatch: [
"**/__tests__/**/*.[jt]s?(x)",
"**/test/**/*.[jt]s?(x)",
"**/?(*.)+(spec|test).[jt]s?(x)"
],
testPathIgnorePatterns: [
'/node_modules/',
'.d.ts$',
'<rootDir>/test/fixtures',
'<rootDir>/test/helpers'
],
transform: {
'^.+\\.[jt]sx?$': 'babel-jest',
},
};
17 changes: 11 additions & 6 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,24 @@
"analyze:dev": "yarn build && yarn analyze",
"analyze:dev:bat": "yarn build && yarn analyze:bat",
"build": "yarn tsc",
"prepublish": "yarn build",
"test": "ava"
"prepublish": "yarn test",
"test": "yarn build && jest"
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't be just "test" : "jest" ?

I tried it and it seems to compile what needed

Copy link
Member Author

Choose a reason for hiding this comment

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

For now, I wanted to have the compilation as a test step. build is NOT the same transformation as jest for now. Jest is relying on babel to strip the types, build is actually compiling via the tsc typescript compiler.

},
"devDependencies": {
"@babel/core": "^7.4.5",
"@babel/preset-env": "^7.4.5",
"@babel/preset-typescript": "^7.3.3",
"@types/jest": "^24.0.13",
"@types/node": "^11.11.6",
"@types/yargs": "^12.0.10",
"ava": "^1.3.1",
"babel-jest": "^24.8.0",
"eslint": "^5.15.3",
"typescript": "^3.3.4000"
"jest": "^24.8.0",
"typescript": "^3.4.5"
},
"dependencies": {
"@typescript-eslint/parser": "^1.5.0",
"@typescript-eslint/typescript-estree": "^1.5.0",
"@typescript-eslint/parser": "^1.9.0",
"@typescript-eslint/typescript-estree": "^1.9.0",
"esm": "^3.2.20",
"yargs": "^13.2.2"
}
Expand Down
4 changes: 2 additions & 2 deletions src/analyzers/base_analyzer.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { parse as parseToTree, ParserOptions } from '@typescript-eslint/typescript-estree'
import { parse as parseToTree, TSESTreeOptions as ParserOptions } from '@typescript-eslint/typescript-estree'
import { Program } from '@typescript-eslint/typescript-estree/dist/ts-estree/ts-estree'

import { Solution } from '../solution'
Expand Down Expand Up @@ -50,7 +50,7 @@ export abstract class BaseAnalyzer {
*
* @memberof BaseAnalyzer
*/
public readonly run = async (): Promise<AnalyzerOutput> => {
public async run(): Promise<AnalyzerOutput> {
await this.execute()
.catch((err) => {
if (err instanceof EarlyFinalization) {
Expand Down
8 changes: 5 additions & 3 deletions src/analyzers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import { Solution } from '../solution'

import { get as getLogger } from '../utils/logger'

import path from 'path'

export class Analyzers {

/**
Expand All @@ -29,13 +31,13 @@ export class Analyzers {
}

private static autoload(exercise: Readonly<Exercise>) {
const path = `./${exercise.slug}/index.js` // explicit path
const modulePath = path.join(__dirname, exercise.slug, 'index') // explicit path (no extension)
try {
return require(path)
return require(modulePath)
} catch(err) {
const logger = getLogger()
logger.error(`
Could not find the index.js analyzer in "${__dirname}/${exercise.slug}"
Could not find the index.js analyzer in "${modulePath}"

Make sure that:
- the slug "${exercise.slug}" is valid (hint: use dashes, not underscores)
Expand Down
4 changes: 1 addition & 3 deletions src/analyzers/utils/extract_export.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import { Program, Node, ExportDefaultDeclaration, ExportNamedDeclaration, ClassDeclaration, VariableDeclaration, ExportDeclaration, ExportSpecifier, AssignmentExpression } from "@typescript-eslint/typescript-estree/dist/ts-estree/ts-estree"
import { AST_NODE_TYPES } from "@typescript-eslint/typescript-estree"
import { traverse, Traverser } from 'eslint/lib/util/traverser'

export { Traverser }
import { traverse } from 'eslint/lib/util/traverser'

export type ExportDeclarationNode = ExportDefaultDeclaration | ExportNamedDeclaration | AssignmentExpression | undefined
export type ExportedNode = ClassDeclaration | VariableDeclaration | ExportDeclaration | ExportSpecifier | undefined
Expand Down
4 changes: 1 addition & 3 deletions src/analyzers/utils/find_all.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import { Node } from "@typescript-eslint/typescript-estree/dist/ts-estree/ts-estree"
import { traverse, Traverser } from 'eslint/lib/util/traverser'

export { Traverser }

/**
* Find all nodes, traversing from root, that match the given predicate
*
Expand All @@ -25,7 +23,7 @@ export { Traverser }
* @param predicate predicate function that gets the traverse as bound this
* @returns the nodes that return true for the predicate
*/
export function findAll(root: Node, predicate: (this: Traverser , node: Node) => boolean): Node[] {
export function findAll(root: Node, predicate: (this: Traverser, node: Node) => boolean): Node[] {
const results: Node[] = []

traverse(root, {
Expand Down
4 changes: 1 addition & 3 deletions src/analyzers/utils/find_first.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { Node } from "@typescript-eslint/typescript-estree/dist/ts-estree/ts-estree"
import { Traverser, traverse } from 'eslint/lib/util/traverser'

export { Traverser }
import { traverse, Traverser } from 'eslint/lib/util/traverser'

/**
* Find the first note (starting at root) that matches the given predicate.
Expand Down
25 changes: 25 additions & 0 deletions test/helpers/bootstrap.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { ExecutionOptions } from "../../src/utils/execution_options";
import { Exercise } from "../../src/exercise";
import { set as setGlobalLogger, Logger } from "../../src/utils/logger";
import { BootstrapResult } from "../../dist/utils/bootstrap";

export function bootstrap({ exercise, ...overrides }: { exercise: string } & Partial<ExecutionOptions>): Omit<BootstrapResult, 'solution'> {
const options = new ExecutionOptions({
debug: false,
console: false,
output: '__fake__',
inputDir: '__fake__',
dry: true,
templates: true,
exercise,
...overrides
})

const logger = setGlobalLogger(new Logger(options))

return {
options,
exercise: new Exercise(exercise),
logger
}
}
29 changes: 29 additions & 0 deletions test/helpers/inline-solution.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { Solution } from "../../src/solution";
import { Exercise } from "../../src/exercise";

export class InlineSolution extends Solution {
/**
* Create a new solution reference
*
* @param rootDir the path to the root directory of the solution
* @param exercise the exercise this solution belongs to
*/
constructor(private readonly solutionFiles: string[], exercise: Exercise) {
super('__fake__', exercise)
}

/**
* Read the solution file(s)
*
* @param n number of files to return
* @returns promise that resolves all the files at once
*/
public async read(n = 1): Promise<Buffer[]> {

return Promise.all(
Object.keys(this.solutionFiles)
.slice(0, n)
.map(name => Buffer.from(this.solutionFiles[name], 'utf8'))
)
}
}
83 changes: 83 additions & 0 deletions test/smoke.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import { Runner } from '../src/runner'
import { Analyzers } from '../src/analyzers'
import { TwoFerAnalyzer } from '../src/analyzers/two-fer'
import { InlineSolution } from './helpers/inline-solution'
import { bootstrap } from './helpers/bootstrap'

const { options, exercise } = bootstrap({ exercise: 'two-fer' })


describe('When running analysis', () => {
it('can approve as optimal', async () => {

const solutionContent = `
export const twoFer = (name = 'you') => {
return \`One for \${name}, one for me.\`;
};
`.trim()

const solution = new InlineSolution([solutionContent], exercise)
Copy link
Contributor

@depsir depsir May 29, 2019

Choose a reason for hiding this comment

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

I like this way to create doubles to make a plain string work with the Analyzer.

I feel the need though to test the analyzer to a lower level, for example to exercise checkStructure without involving the solution, the exercise, the runner

I would love something like

    const solutionContent = `
    export const somethingElse = (name = 'you') => {
      return \`One for \${name}, one for me.\`;
    };
    `.trim()

    const program = parseToTree(solutionContent);
    const analyzer = new TwoFerAnalyzer();
    analyzer.program = program;
    expect(analyzer.checkStructure()).toBe("DisapproveWithComment")

Probably this should be discussed in #16 since it involves changing the structure of the analyzer, but I feel that a minimal smoke test for a simple function should be available in this pr

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel the need though to test the analyzer to a lower level, for example to exercise checkStructure without involving the solution, the exercise, the runner

In short: this is possible 👍, using an InlineSolution or something that conforms to the Solution interface:

const analyzerUnderTest = new Analyzer(somethingThatsASolution)
expect(analyzer.method()).toBe(expectedResult)

// Or run it as a whole
const output = analyzerUnderTest.run()
// => AnalyzerOutput({ status: 'some-status', comments: Comment[] })

InlineSolution is really just a ValueObject and passing around strings is Primitive Obsession. I don't think any test benefits from passing around strings 👎 . You can create another test class or method on InlineSolution or global method:

InlineSolution.from(programString)
// => a solution

createSolutionFromString(programString)
// => a solution

The right move forward is probably turning Solution into an interface and create FileSolution and InlineSolution, completely removing the hierarchy, which I only added because I was being lazy 😅

const program = parseToTree(solutionContent);
const analyzer = new TwoFerAnalyzer();
analyzer.program = program;

This is not a good idea:

  • requires exposing program which is an internal state that has nothing to do with the goal of the execution which is coupling implementation and not testing behaviour
  • assumes that we must parse in order to analyse (which is again testing implementation and not behaviour); there are plenty of valid ways to analyse code and an AST might not be applicable for all JavaScript or TypeScript analyzers -- and I don't like to couple the test in that manner.
  • couples the parseToTree with the test; currently this is called only at one single entry point so we can switch out the implementation; but this could be overcome by re-exporting the parsing entry in a local file which is ok.

That said: Running the analyzer without a runner is possible, and we should test it; I will add the tests when testing the analyzer base class -- but not the smoke test. The smoke test is to detect fire, and it should be an reasonably complete e2e test, hitting all the parts. If this fails, something, somewhere, is probably wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

@depsir have a look at #18

Copy link
Contributor

@depsir depsir May 30, 2019

Choose a reason for hiding this comment

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

I like #18. I think you took the good points from my comment above and put them into code.. I would like to clarify some points anyway

InlineSolution is really just a ValueObject and passing around strings is Primitive Obsession. I don't think any test benefits from passing around strings

Not sure about the definition of value object, I would think to some kind of pojo, but the solution has that read method. Not sure about primitive obsession too, in javascript/typescript but I understand its value.
In the end, you did what i meant in #18, where the analyzer takes the string (wrapped in an Input object) as Input directly, without bootstrap, options, solution and so on

requires exposing program which is an internal state that has nothing to do with the goal of the execution which is coupling implementation and not testing behaviour

yes, it was a bad way to express that i wanted the analyzer to take an ast as input. I tried to use what was available in the current api, even though private. I'm not sure that having an ast should be an implementation detail but I can live with that.

just to be clear for any other reviewer, if we have a very simple program to validate (export const color = "blue") we could in theory use a regex without involving a parser. I cannot find other examples where the parser is not needed but It makes [some] sense

About testing the behavior and not the implementation, I think that at some point we will need to unit test some steps of the analysis, e.g. checkForSolutionWithoutStringTemplate of two fer, and this will probably need to setup the analyzer with an already parsed example, i.e. we cannot provide the input and then call execute to parse it, just to test checkForSolutionWithoutStringTemplate.
But as you said, this is a smoke test, where we want to exercise the entire behavior on some of the possible paths. That's why I referred #16 as more appropriate to discuss these structural changes

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about the definition of value object, I would think to some kind of pojo, but the solution has that read method.

Yes, Value Objects are just identified by their value but allow you to attach conversion/casting methods, and use instanceof, implements/extends and so forth, so basically a decorated primitive or set of primitives (or other value objects).

just to be clear for any other reviewer, if we have a very simple program to validate (export const color = "blue") we could in theory use a regex without involving a parser. I cannot find other examples where the parser is not needed but It makes [some] sense

Correct ✔️

const analyzer = new TwoFerAnalyzer(solution)

const output = await Runner.call(analyzer, options);
expect(output.status).toBe('approve_as_optimal');
expect(output.comments.length).toBe(0);
})

it('can approve with comment', async () => {

const solutionContent = `
const twoFer = (name = 'you') => {
return \`One for \${name}, one for me.\`;
};

export { twoFer }
`.trim()

const solution = new InlineSolution([solutionContent], exercise)
const analyzer = new TwoFerAnalyzer(solution)

const output = await Runner.call(analyzer, options);
expect(output.status).toBe('approve_with_comment');
expect(output.comments.length).toBeGreaterThanOrEqual(1);
})

it('can dissapprove with comment', async () => {

const solutionContent = `
export const twoFer = (name) => {
return \`One for \${name || 'you'}, one for me.\`;
};
`.trim()

const solution = new InlineSolution([solutionContent], exercise)
const analyzer = new TwoFerAnalyzer(solution)

const output = await Runner.call(analyzer, options);
expect(output.status).toBe('disapprove_with_comment');
expect(output.comments.length).toBeGreaterThanOrEqual(1);
})

it('can refer to mentor', async () => {

const solutionContent = `
const whomst = 'for'
export const twoFer = (name = 'you') => {
return \`One \${whomst} \${name}, one \${whomst} me.\`;
};
`.trim()

const solution = new InlineSolution([solutionContent], exercise)
const analyzer = new TwoFerAnalyzer(solution)

const output = await Runner.call(analyzer, options);
expect(output.status).toBe('refer_to_mentor');
})
})

describe('When autoloading analyzers', () => {
it('can find an analyzer based on an exercise', () => {
const ActualAnalyzer = Analyzers.find(exercise)
expect(ActualAnalyzer).toBe(TwoFerAnalyzer)
})
})
1 change: 1 addition & 0 deletions test/types.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
type Omit<T, K> = Pick<T, Exclude<keyof T, K>>;
10 changes: 5 additions & 5 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
// "allowJs": true, /* Allow javascript files to be compiled. */
// "checkJs": true, /* Report errors in .js files. */
// "jsx": "preserve", /* Specify JSX code generation: 'preserve', 'react-native', or 'react'. */
"declaration": true, /* Generates corresponding '.d.ts' file. */
"declarationMap": true, /* Generates a sourcemap for each corresponding '.d.ts' file. */
// "declaration": true, /* Generates corresponding '.d.ts' file. */
// "declarationMap": true, /* Generates a sourcemap for each corresponding '.d.ts' file. */
// "sourceMap": true, /* Generates corresponding '.map' file. */
// "outFile": "./", /* Concatenate and emit output to single file. */
"outDir": "./dist", /* Redirect output structure to the directory. */
Expand All @@ -18,7 +18,7 @@
// "noEmit": true, /* Do not emit outputs. */
// "importHelpers": true, /* Import emit helpers from 'tslib'. */
// "downlevelIteration": true, /* Provide full support for iterables in 'for-of', spread, and destructuring when targeting 'ES5' or 'ES3'. */
// "isolatedModules": true, /* Transpile each file as a separate module (similar to 'ts.transpileModule'). */
"isolatedModules": true, /* Transpile each file as a separate module (similar to 'ts.transpileModule'). */

/* Strict Type-Checking Options */
"strict": true, /* Enable all strict type-checking options. */
Expand Down Expand Up @@ -57,6 +57,6 @@
// "experimentalDecorators": true, /* Enables experimental support for ES7 decorators. */
// "emitDecoratorMetadata": true, /* Enables experimental support for emitting type metadata for decorators. */
},
"include": ["src"],
"exclude": ["test/solutions"]
"include": ["src", "declarations.d.ts", "types.d.ts"],
"exclude": ["test/fixtures"]
}
Loading