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

Mejora comportamiento comando test con parametros -f -d -t #162

Merged
merged 4 commits into from
Jun 15, 2024
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
44 changes: 34 additions & 10 deletions src/commands/test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { bold } from 'chalk'
import { bold, red } from 'chalk'
import { time, timeEnd } from 'console'
import logger from 'loglevel'
import { Entity, Environment, Node, Test, is, match, when, WRENatives as natives, interpret, Describe } from 'wollok-ts'
Expand All @@ -17,32 +17,54 @@ export type Options = {
skipValidations: boolean
}

class TestSearchMissError extends Error{}

export function validateParameters(filter: string | undefined, { file, describe, test }: Options): void {
if (filter && (file || describe || test)) throw new Error('You should either use filter by full name or file/describe/test.')
}

export function matchingTestDescription(filter: string | undefined, options: Options): string {
if(filter) return `matching ${valueDescription(filter)}`
if(options.file || options.describe || options.test) {
const stringifiedOrWildcard = (value?: string) => value ? `'${value}'` : '*'
return `matching ${valueDescription([options.file, options.describe, options.test].map(stringifiedOrWildcard).join('.'))}`
}
return ''
}

export function sanitize(value?: string): string | undefined {
return value?.replaceAll('"', '')
}

export function getTarget(environment: Environment, filter: string | undefined, options: Options): Test[] {
const possibleTargets = getBaseNode(environment, filter, options).descendants.filter(getTestFilter(filter, options))
const onlyTarget = possibleTargets.find((test: Test) => test.isOnly)
const testMatches = (filter: string) => (test: Test) => !filter || sanitize(test.fullyQualifiedName)!.includes(filter)
const filterTest = sanitize(filter) ?? ''
return onlyTarget ? [onlyTarget] : possibleTargets.filter(testMatches(filterTest))
let possibleTargets: Test[]
try {
possibleTargets = getBaseNode(environment, filter, options).descendants.filter(getTestFilter(filter, options))
const onlyTarget = possibleTargets.find((test: Test) => test.isOnly)
Copy link
Contributor

Choose a reason for hiding this comment

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

lo aprobé y me quedé pensando si por algún motivo no pusiste el resto del código acá, o bien si no querés llamar a otra función interna...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No 😅, la verdad que no lo pense tanto, como el resto del codigo no podia generar ese error no me cambiaba mucho. Lo muevo adentro del try.

o bien si no querés llamar a otra función interna...

Uh no entendi dodain, que funcion llamaria?

Copy link
Contributor

Choose a reason for hiding this comment

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

sería algo como todo lo que iría en el try

try {
   xxx() // función que obtiene possibleTargets y hace todo lo que ahora está post-catch
} catch ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh okay ya entendi

No tengo una opinion fuerte al respecto pero esa funcion xxx no seria simplemente getTarget? Que ganamos que moverlo a otra funcion? Y que nombre le pondriamos si no es getTarget?

Copy link
Contributor

Choose a reason for hiding this comment

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

Lo que ganás generalmente es que evitás el problema de la indentación anidada, no mucho pero si el método tiene 88 líneas es medio un embole poder capturar el try/catch cuando lo estás leyendo (por supuesto, podés colapsarlo). Igual no digo que hagas un método privado, lo que sí no me gusta es tener un try/catch específico para eso a menos que quieras que cualquier otro error salte.

const testMatches = (filter: string) => (test: Test) => !filter || sanitize(test.fullyQualifiedName)!.includes(filter)
const filterTest = sanitize(filter) ?? ''
return onlyTarget ? [onlyTarget] : possibleTargets.filter(testMatches(filterTest))
} catch(e: any){
if(e instanceof TestSearchMissError){
logger.error(red(bold(e.message)))
return []
}
throw e
}
}

function getBaseNode(environment: Environment, filter: string | undefined, options: Options): Environment | Package | Describe {
if (filter) return environment

const { file, describe } = options
let nodeToFilter: Environment | Package | Describe = environment
let nodeToFilter: Environment | Package | Describe | undefined = environment
if (file) {
nodeToFilter = nodeToFilter.descendants.find(node => node.is(Package) && node.name === file) as Package | undefined ?? nodeToFilter
nodeToFilter = nodeToFilter.descendants.find(node => node.is(Package) && node.fileName === file) as Package | undefined
if(!nodeToFilter) throw new TestSearchMissError(`File '${file}' not found`)
}
if (describe) {
nodeToFilter = nodeToFilter.descendants.find(node => node.is(Describe) && node.name === `"${describe}"`) as Describe | undefined ?? nodeToFilter
nodeToFilter = nodeToFilter.descendants.find(node => node.is(Describe) && node.name === `"${describe}"`) as Describe | undefined
if(!nodeToFilter) throw new TestSearchMissError(`Describe '${describe}' not found`)
}
return nodeToFilter
}
Expand All @@ -62,7 +84,9 @@ export default async function (filter: string | undefined, options: Options): Pr

const timeMeasurer = new TimeMeasurer()
const { project, skipValidations } = options
const runAllTestsDescription = `${testIcon} Running all tests ${filter ? `matching ${valueDescription(filter)} ` : ''}on ${valueDescription(project)}`

const matchLog = matchingTestDescription(filter, options)
const runAllTestsDescription = `${testIcon} Running all tests${matchLog ? ` ${matchLog} `: ' '}on ${valueDescription(project)}`

logger.info(runAllTestsDescription)

Expand Down
2 changes: 1 addition & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ program.command('test')
.description('Run Wollok tests')
.argument('[filter]', 'filter pattern for a test, describe or package')
.option('-p, --project [project]', 'path to project', process.cwd())
.option('-f, --file [file]', 'path to file', '')
.option('-f, --file [file]', 'path to file relative to the project', '')
.option('-d, --describe [describe]', 'describe to run', '')
.option('-t, --test [test]', 'test to run', '')
.option('--skipValidations', 'skip code validation', false)
Expand Down
80 changes: 71 additions & 9 deletions test/test.test.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { expect } from 'chai'
import logger from 'loglevel'
import { join } from 'path'
import { buildEnvironmentForProject } from '../src/utils'
import test, { getTarget, sanitize, tabulationForNode, validateParameters } from '../src/commands/test'
import sinon from 'sinon'
import { Environment } from 'wollok-ts'
import logger from 'loglevel'
import test, { getTarget, matchingTestDescription, sanitize, tabulationForNode, validateParameters } from '../src/commands/test'
import { logger as fileLogger } from '../src/logger'
import sinon from 'sinon'
import { buildEnvironmentForProject } from '../src/utils'
import { spyCalledWithSubstring } from './assertions'

describe('Test', () => {
Expand Down Expand Up @@ -121,7 +121,7 @@ describe('Test', () => {
it('should filter by file using file option', () => {
const tests = getTarget(environment, undefined, {
...emptyOptions,
file: 'test-one',
file: 'test-one.wtest',
})
expect(tests.length).to.equal(3)
expect(tests[0].name).to.equal('"a test"')
Expand All @@ -132,7 +132,7 @@ describe('Test', () => {
it('should filter by file & describe using file & describe option', () => {
const tests = getTarget(environment, undefined, {
...emptyOptions,
file: 'test-one',
file: 'test-one.wtest',
describe: 'this describe',
})
expect(tests.length).to.equal(3)
Expand All @@ -144,7 +144,7 @@ describe('Test', () => {
it('should filter by file & describe & test using file & describe & test option', () => {
const tests = getTarget(environment, undefined, {
...emptyOptions,
file: 'test-one',
file: 'test-one.wtest',
describe: 'this describe',
test: 'another test',
})
Expand Down Expand Up @@ -195,7 +195,7 @@ describe('Test', () => {
it('should execute single test when running a file using file option', () => {
const tests = getTarget(environment, undefined, {
...emptyOptions,
file: 'only-file',
file: 'only-file.wtest',
})
expect(tests.length).to.equal(1)
expect(tests[0].name).to.equal('"this is the one"')
Expand Down Expand Up @@ -250,6 +250,43 @@ describe('Test', () => {

})

describe('matching test description', () => {
const emptyOptions = {
project: '',
skipValidations: false,
file: undefined,
describe: undefined,
test: undefined,
}


it('should return empty string if no filter or options are passed', () => {
expect(matchingTestDescription(undefined, emptyOptions)).to.equal('')
})

it('should return filter description if filter is passed', () => {
expect(matchingTestDescription('some test', emptyOptions)).to.include('some test')
})

it('should return options descriptions if options are passed', () => {
expect(matchingTestDescription(undefined, {
...emptyOptions,
file: 'test-one.wtest',
describe: 'this describe',
test: 'another test',
})).to.include('\'test-one.wtest\'.\'this describe\'.\'another test\'')
})

it('should return options descriptions with wildcards if options are missing', () => {
expect(matchingTestDescription(undefined, {
...emptyOptions,
file: undefined,
describe: 'this discribe',
test: undefined,
})).to.include('*.\'this discribe\'.*')
})
})

describe('tabulations for node', () => {

it('should work for root package', () => {
Expand All @@ -265,6 +302,7 @@ describe('Test', () => {

let fileLoggerInfoSpy: sinon.SinonStub
let loggerInfoSpy: sinon.SinonStub
let loggerErrorSpy: sinon.SinonStub
let processExitSpy: sinon.SinonStub

const projectPath = join('examples', 'test-examples', 'normal-case')
Expand All @@ -281,6 +319,7 @@ describe('Test', () => {
loggerInfoSpy = sinon.stub(logger, 'info')
fileLoggerInfoSpy = sinon.stub(fileLogger, 'info')
processExitSpy = sinon.stub(process, 'exit')
loggerErrorSpy = sinon.stub(logger, 'error')
})

afterEach(() => {
Expand All @@ -290,7 +329,7 @@ describe('Test', () => {
it('passes all the tests successfully and exits normally', async () => {
await test(undefined, {
...emptyOptions,
file: 'test-one',
file: 'test-one.wtest',
})

expect(processExitSpy.callCount).to.equal(0)
Expand All @@ -301,6 +340,29 @@ describe('Test', () => {
expect(fileLoggerInfoSpy.firstCall.firstArg.result).to.deep.equal({ ok: 3, failed: 0 })
})

it('passing a wrong filename runs no tests and logs a warning', async () => {
await test(undefined, {
...emptyOptions,
file: 'non-existing-file.wtest',
})

expect(processExitSpy.callCount).to.equal(0)
expect(spyCalledWithSubstring(loggerInfoSpy, 'Running 0 tests')).to.be.true
expect(spyCalledWithSubstring(loggerErrorSpy, 'File \'non-existing-file.wtest\' not found')).to.be.true
})

it('passing a wrong describe runs no tests and logs a warning', async () => {
await test(undefined, {
...emptyOptions,
file: 'test-one.wtest',
describe: 'non-existing-describe',
})

expect(processExitSpy.callCount).to.equal(0)
expect(spyCalledWithSubstring(loggerInfoSpy, 'Running 0 tests')).to.be.true
expect(spyCalledWithSubstring(loggerErrorSpy, 'Describe \'non-existing-describe\' not found')).to.be.true
})

it('returns exit code 2 if one or more tests fail', async () => {
await test(undefined, emptyOptions)

Expand Down
Loading