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

error origins #529

Merged
merged 30 commits into from
Oct 12, 2020
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
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
5 changes: 3 additions & 2 deletions jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ module.exports = {
// A set of global variables that need to be available in all test environments
globals: {
"ts-jest": {
"tsConfig": "tsconfig.jest.json"
"tsConfig": "./test/tsconfig.json"
}
},

Expand All @@ -32,7 +32,8 @@ module.exports = {

// The paths to modules that run some code to configure or set up the testing environment after each test
setupFilesAfterEnv: [
'<rootDir>/test/_setupFiles/bootstrap.ts'
'<rootDir>/test/_setupFiles/bootstrap.ts',
'<rootDir>/test/_setupFiles/jest/bootstrap.ts'
],

// The test environment that will be used for testing
Expand Down
2 changes: 1 addition & 1 deletion src/BuildEngineFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export class BuildEngineFactory {
lazilyTransformingAstService.undoRedo = crudOperations.undoRedo
lazilyTransformingAstService.parser = parser

const exporter = new Exporter(config, namedExpressions)
const exporter = new Exporter(config, namedExpressions, sheetMapping.fetchDisplayName)
const serialization = new Serialization(dependencyGraph, unparser, config, exporter)

const evaluator = new Evaluator(dependencyGraph, columnSearch, config, stats, dateHelper, numberLiteralHelper, functionRegistry, namedExpressions, serialization)
Expand Down
1 change: 1 addition & 0 deletions src/Cell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ export class CellError {
constructor(
public readonly type: ErrorType,
public readonly message?: string,
public address?: SimpleCellAddress,
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO shouldn't be optional, and should be readonly. Set once and propagated. Do we need to edit it later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we need to edit it later?

Yes, errors are sometimes created in a context that lacks address, and this value is supplemented later.
Right now this address is set in a single place in an interpreter.

) {
}

Expand Down
14 changes: 12 additions & 2 deletions src/CellValue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {CellValueChange} from './ContentChanges'
import {ErrorMessage} from './error-message'
import {NamedExpressions} from './NamedExpressions'
import {InterpreterValue, SimpleRangeValue} from './interpreter/InterpreterValue'
import {SheetIndexMappingFn, simpleCellAddressToString} from './parser/addressRepresentationConverters'

export type NoErrorCellValue = number | string | boolean | null
export type CellValue = NoErrorCellValue | DetailedCellError
Expand Down Expand Up @@ -53,10 +54,10 @@ export class ExportedNamedExpressionChange {
export class DetailedCellError {
public readonly type: ErrorType
public readonly message: string

constructor(
error: CellError,
public readonly value: string,
public readonly address?: string,
) {
this.type = error.type
this.message = error.message || ''
Expand All @@ -67,6 +68,7 @@ export class Exporter {
constructor(
private readonly config: Config,
private readonly namedExpressions: NamedExpressions,
private readonly sheetIndexMapping: SheetIndexMappingFn,
) {
}

Expand Down Expand Up @@ -103,7 +105,15 @@ export class Exporter {
}

private detailedError(error: CellError): DetailedCellError {
return new DetailedCellError(error, this.config.translationPackage.getErrorTranslation(error.type))
let address = undefined
if(error.address !== undefined) {
if (error.address.sheet === NamedExpressions.SHEET_FOR_WORKBOOK_EXPRESSIONS) {
address = this.namedExpressions.namedExpressionInAddress(error.address.row)?.displayName
} else {
address = simpleCellAddressToString(this.sheetIndexMapping, error.address, -1)
}
}
return new DetailedCellError(error, this.config.translationPackage.getErrorTranslation(error.type), address)
izulin marked this conversation as resolved.
Show resolved Hide resolved
}

private cellValueRounding(value: number): number {
Expand Down
17 changes: 11 additions & 6 deletions src/interpreter/Interpreter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,24 +54,23 @@ export class Interpreter {
}

public evaluateAst(ast: Ast, formulaAddress: SimpleCellAddress): InterpreterValue {
const val = this.evaluateAstWithoutPostoprocessing(ast, formulaAddress)
let val = this.evaluateAstWithoutPostoprocessing(ast, formulaAddress)
if (typeof val === 'number') {
if (isNumberOverflow(val)) {
return new CellError(ErrorType.NUM, ErrorMessage.NaN)
val = new CellError(ErrorType.NUM, ErrorMessage.NaN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the wrapperForAddress reason why the error address is not private? We could pass it to the constructor right here without wrapping it later

Suggested change
val = new CellError(ErrorType.NUM, ErrorMessage.NaN)
return new CellError(ErrorType.NUM, ErrorMessage.NaN, formulaAddress)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, but the reason behind wrapperForAddress is to fix whatever comes from evaluation, even in nested, recursive calls

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 all functions return the correct error? We're patching the error object just to avoid fixing this in the right places or there is some way to create an error that don't know it's origin?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've refactored it slightly to make the field private.
To answer your question:

  • I'm not sure whether in every place the constructor of CellError is called, we have formulaAddress in the context (there might be some obscure case that would require ugly refactor)
  • Adding it here, in a single place, is a minimal change to code that made this feature work, and thus it's much easier to make sure the logic is correctly added in every evaluation.

Copy link
Contributor

Choose a reason for hiding this comment

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

getter/setter are ok, but my main idea was that CellError is immutable.

Once created it cannot be altered, especially the origin address. Having this editable is IMO a risk.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not getter/setter, its getter and (setter that only sets it once, if the value is not present yet). So I don't see any risk here. Also, keep in mind that this value is supplementary one, it does not affect outcome of calculation, only provides debugging info.

} else {
return fixNegativeZero(val)
val = fixNegativeZero(val)
}
} else {
return val
}
return wrapperForAddress(val, formulaAddress)
}
/**
* Calculates cell value from formula abstract syntax tree
*
* @param formula - abstract syntax tree of formula
* @param formulaAddress - address of the cell in which formula is located
*/
public evaluateAstWithoutPostoprocessing(ast: Ast, formulaAddress: SimpleCellAddress): InterpreterValue {
private evaluateAstWithoutPostoprocessing(ast: Ast, formulaAddress: SimpleCellAddress): InterpreterValue {
switch (ast.type) {
case AstNodeType.EMPTY: {
return EmptyValue
Expand Down Expand Up @@ -322,3 +321,9 @@ function wrapperBinary<T extends InterpreterValue>(op: (a: T, b: T) => Interpret
}
}

function wrapperForAddress(val: InterpreterValue, adr: SimpleCellAddress): InterpreterValue {
if(val instanceof CellError) {
val.address = val.address ?? adr
}
return val
}
14 changes: 8 additions & 6 deletions test/CellValueExporter.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,17 @@ import {CellError, EmptyValue, ErrorType} from '../src/Cell'
import {Exporter} from '../src/CellValue'
import {Config} from '../src/Config'
import {plPL} from '../src/i18n/languages'
import {SheetIndexMappingFn} from '../src/parser/addressRepresentationConverters'
import {detailedError} from './testUtils'
import {NamedExpressions} from '../src/NamedExpressions'

const namedExpressionsMock = {} as NamedExpressions
const sheetIndexMock = {} as SheetIndexMappingFn

describe( 'rounding', () => {
it( 'no rounding', () =>{
const config = new Config({ smartRounding : false })
const cellValueExporter = new Exporter(config, namedExpressionsMock)
const cellValueExporter = new Exporter(config, namedExpressionsMock, sheetIndexMock)
expect(cellValueExporter.exportValue(1.000000000000001)).toBe(1.000000000000001)
expect(cellValueExporter.exportValue(-1.000000000000001)).toBe(-1.000000000000001)
expect(cellValueExporter.exportValue(0.000000000000001)).toBe(0.000000000000001)
Expand All @@ -25,7 +27,7 @@ describe( 'rounding', () => {

it( 'with rounding', () =>{
const config = new Config()
const cellValueExporter = new Exporter(config, namedExpressionsMock)
const cellValueExporter = new Exporter(config, namedExpressionsMock, sheetIndexMock)
expect(cellValueExporter.exportValue(1.0000000000001)).toBe(1.0000000000001)
expect(cellValueExporter.exportValue(-1.0000000000001)).toBe(-1.0000000000001)
expect(cellValueExporter.exportValue(1.000000000000001)).toBe(1)
Expand All @@ -43,20 +45,20 @@ describe( 'rounding', () => {
describe('detailed error', () => {
it('should return detailed errors', () => {
const config = new Config({ language: 'enGB' })
const cellValueExporter = new Exporter(config, namedExpressionsMock)
const cellValueExporter = new Exporter(config, namedExpressionsMock, sheetIndexMock)

const error = cellValueExporter.exportValue(new CellError(ErrorType.VALUE)) as DetailedCellError
expect(error).toEqual(detailedError(ErrorType.VALUE))
expect(error).toEqualError(detailedError(ErrorType.VALUE))
expect(error.value).toEqual('#VALUE!')
})

it('should return detailed errors with translation', () => {
HyperFormula.registerLanguage('plPL', plPL)
const config = new Config({ language: 'plPL' })
const cellValueExporter = new Exporter(config, namedExpressionsMock)
const cellValueExporter = new Exporter(config, namedExpressionsMock, sheetIndexMock)

const error = cellValueExporter.exportValue(new CellError(ErrorType.VALUE)) as DetailedCellError
expect(error).toEqual(detailedError(ErrorType.VALUE, undefined, config))
expect(error).toEqualError(detailedError(ErrorType.VALUE, undefined, config))
expect(error.value).toEqual('#ARG!')
})
})
9 changes: 3 additions & 6 deletions test/_setupFiles/bootstrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {Config} from '../../src/Config'
import {AlwaysSparse} from '../../src/DependencyGraph/AddressMapping/ChooseAddressMappingPolicy'
import {enGB} from '../../src/i18n/languages'
import {unregisterAllLanguages} from './../testUtils'
import {toContainEqualMatcher, toMatchObjectMatcher} from './matchers'
import {toContainEqualMatcher, toEqualErrorMatcher, toMatchObjectMatcher} from './matchers'
import * as plugins from '../../src/interpreter/plugin'

Config.defaultConfig = Object.assign({}, Config.defaultConfig, {
Expand Down Expand Up @@ -53,14 +53,11 @@ beforeEach(() => {
})

beforeAll(() => {
if(jestPresent) {
// eslint-disable-next-line @typescript-eslint/ban-ts-ignore
// @ts-ignore
spyOn = jest.spyOn
} else {
if(!jestPresent) {
jasmine.addMatchers({
...toContainEqualMatcher,
...toMatchObjectMatcher,
...toEqualErrorMatcher,
})
}
})
8 changes: 8 additions & 0 deletions test/_setupFiles/jest/bootstrap.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import {toEqualError} from './toEqualError'

beforeAll(() => {
// eslint-disable-next-line @typescript-eslint/ban-ts-ignore
// @ts-ignore
spyOn = jest.spyOn
expect.extend(toEqualError)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we add this to the main bootstrap.ts? Now we have two, one clearly marked as jest the other unknown. We should follow a convention one way or another.

CC @swistach

Copy link
Collaborator

@voodoo11 voodoo11 Oct 5, 2020

Choose a reason for hiding this comment

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

I know that present setup looks a littlebit strange, but there were some problems and type definition clashes otherwise. If you now how to simplify this setup to be able to work locally with jest in IntelliJ and debugger and not break jasmine at the same time, feel free to correct it. I failed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Discussed offline. I will try to describe problem behind it in PR description.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wojciechczerniak I've added reminders in tsconfigs and updated PR description.

27 changes: 27 additions & 0 deletions test/_setupFiles/jest/toEqualError.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
type CustomMatcherResult = jest.CustomMatcherResult
type ExpectExtendMap = jest.ExpectExtendMap

declare global {
namespace jest {
interface Matchers<R, T> {
toEqualError(expected: any): CustomMatcherResult,
}
}
}

export const toEqualError: ExpectExtendMap = {toEqualError(received: any, expected: any): CustomMatcherResult {
let result = false
izulin marked this conversation as resolved.
Show resolved Hide resolved
if (typeof received === 'object' && typeof expected === 'object') {
result = this.equals(
{...received, address: undefined},
{...expected, address: undefined}
)
} else {
result = this.equals(received, expected)
}
return {
pass: result,
message: () => (result ? '' : `Expected ${received} to be match ${expected}.`)
}
}
}
1 change: 1 addition & 0 deletions test/_setupFiles/matchers/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export { toContainEqualMatcher } from './toContainEqual'
export { toMatchObjectMatcher } from './toMatchObject'
export { toEqualErrorMatcher } from './toEqualError'
34 changes: 34 additions & 0 deletions test/_setupFiles/matchers/toEqualError.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
type CustomMatcher = jasmine.CustomMatcher
type CustomMatcherFactories = jasmine.CustomMatcherFactories
type CustomMatcherResult = jasmine.CustomMatcherResult
type MatchersUtil = jasmine.MatchersUtil

declare global {
namespace jasmine {
interface Matchers<T> {
toEqualError(expected: any, expectationFailOutput?: string): boolean,
}
}
}

export const toEqualErrorMatcher: CustomMatcherFactories = {
toEqualError: function(util: MatchersUtil): CustomMatcher {
return {
compare: function(received: any, expected: any): CustomMatcherResult {
let result
if (typeof received === 'object' && typeof expected === 'object') {
result = util.equals(
{...received, address: undefined},
{...expected, address: undefined}
)
} else {
result = util.equals(received, expected)
}
return {
pass: result,
message: result ? '' : `Expected ${JSON.stringify(received, null, 2)} to match ${JSON.stringify(expected, null, 2)}.`
}
},
}
}
}
4 changes: 2 additions & 2 deletions test/criterion.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {ErrorType} from '../src/Cell'
import {CellError, ErrorType} from '../src/Cell'
import {Config} from '../src/Config'
import {DateTimeHelper} from '../src/DateTimeHelper'
import {ArithmeticHelper} from '../src/interpreter/ArithmeticHelper'
Expand Down Expand Up @@ -70,7 +70,7 @@ describe('Criterion', () => {
})

it('null when criterion being error', () => {
expect(criterionBuilder.parseCriterion(detailedError(ErrorType.VALUE), arithmeticHelper)).toEqual(undefined)
expect(criterionBuilder.parseCriterion(new CellError(ErrorType.VALUE), arithmeticHelper)).toEqual(undefined)
})

it('works with criterion being just value', () => {
Expand Down
14 changes: 7 additions & 7 deletions test/cruds/change-cell-content.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ describe('changing cell content', () => {

engine.setCellContents(adr('A1'), '#DIV/0!')

expect(engine.getCellValue(adr('A1'))).toEqual(detailedError(ErrorType.DIV_BY_ZERO))
expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.DIV_BY_ZERO))
})

it('update value cell to error-like literal', () => {
Expand All @@ -227,7 +227,7 @@ describe('changing cell content', () => {

engine.setCellContents(adr('A1'), '=SUM(')

expect(engine.getCellValue(adr('A1'))).toEqual(detailedError(ErrorType.ERROR, ErrorMessage.ParseError))
expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.ERROR, ErrorMessage.ParseError))
expect(engine.getCellFormula(adr('A1'))).toEqual('=SUM(')
})

Expand Down Expand Up @@ -526,7 +526,7 @@ describe('changing cell content', () => {

engine.setCellContents(adr('A1'), '=SUM(')

expect(engine.getCellValue(adr('A1'))).toEqual(detailedError(ErrorType.ERROR, ErrorMessage.ParseError))
expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.ERROR, ErrorMessage.ParseError))
})

it('update dependecy value cell to parsing error ', () => {
Expand All @@ -540,8 +540,8 @@ describe('changing cell content', () => {
const a1 = engine.addressMapping.fetchCell(adr('A1'))
const b1 = engine.addressMapping.fetchCell(adr('B1'))
expect(engine.graph.existsEdge(a1, b1)).toBe(true)
expect(engine.getCellValue(adr('A1'))).toEqual(detailedError(ErrorType.ERROR, ErrorMessage.ParseError))
expect(engine.getCellValue(adr('B1'))).toEqual(detailedError(ErrorType.ERROR, ErrorMessage.ParseError))
expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.ERROR, ErrorMessage.ParseError))
expect(engine.getCellValue(adr('B1'))).toEqualError(detailedError(ErrorType.ERROR, ErrorMessage.ParseError))
})

it('update formula cell to parsing error ', () => {
Expand All @@ -557,7 +557,7 @@ describe('changing cell content', () => {
expect(engine.graph.existsEdge(a1, b1)).toBe(false)

expect(engine.getCellValue(adr('A1'))).toEqual(1)
expect(engine.getCellValue(adr('B1'))).toEqual(detailedError(ErrorType.ERROR, ErrorMessage.ParseError))
expect(engine.getCellValue(adr('B1'))).toEqualError(detailedError(ErrorType.ERROR, ErrorMessage.ParseError))
})

it('update parsing error to formula', () => {
Expand All @@ -576,7 +576,7 @@ describe('changing cell content', () => {

engine.setCellContents(adr('A1'), '{=TRANSPOSE(}')

expect(engine.getCellValue(adr('A1'))).toEqual(detailedError(ErrorType.ERROR, ErrorMessage.ParseError))
expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.ERROR, ErrorMessage.ParseError))
expect(engine.getCellFormula(adr('A1'))).toEqual('{=TRANSPOSE(}')
})

Expand Down
2 changes: 1 addition & 1 deletion test/cruds/copy-paste.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ describe('Copy - paste integration', () => {
engine.copy(adr('B2'), 1, 1)
engine.paste(adr('A1'))

expect(engine.getCellValue(adr('A1'))).toEqual(detailedError(ErrorType.REF, ErrorMessage.BadRef))
expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.REF, ErrorMessage.BadRef))
})

it('should create new range vertex - cell range', () => {
Expand Down
8 changes: 4 additions & 4 deletions test/cruds/cut-paste.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ describe('Address dependencies, moved formulas', () => {
engine.cut(adr('A1'), 1, 4)
engine.paste(adr('B1'))

expect(engine.getCellValue(adr('B1'))).toEqual(detailedError(ErrorType.CYCLE))
expect(engine.getCellValue(adr('B2'))).toEqual(detailedError(ErrorType.CYCLE))
expect(engine.getCellValue(adr('B3'))).toEqual(detailedError(ErrorType.CYCLE))
expect(engine.getCellValue(adr('B4'))).toEqual(detailedError(ErrorType.CYCLE))
expect(engine.getCellValue(adr('B1'))).toEqualError(detailedError(ErrorType.CYCLE))
expect(engine.getCellValue(adr('B2'))).toEqualError(detailedError(ErrorType.CYCLE))
expect(engine.getCellValue(adr('B3'))).toEqualError(detailedError(ErrorType.CYCLE))
expect(engine.getCellValue(adr('B4'))).toEqualError(detailedError(ErrorType.CYCLE))
expect(engine.getCellFormula(adr('B1'))).toEqual('=B1')
expect(engine.getCellFormula(adr('B2'))).toEqual('=$B2')
expect(engine.getCellFormula(adr('B3'))).toEqual('=B$3')
Expand Down
Loading