error origins #529

izulin · 2020-09-23T11:51:19Z

Context

Errors should preserve the original cell that triggers them.

How has this been tested?

Adds a new feature: CellError type carries the address of the original cell that produced the error.

Additionally, exporting/serialization produces nicely human-readable addresses. So we can have following tests:

    const engine = HyperFormula.buildFromArray([
      ['=NA()', '=A1']
    ])
    expect(engine.getCellValue(adr('A1'))).toEqual(detailedErrorWithOrigin(ErrorType.NA, 'Sheet1!A1'))
    expect(engine.getCellValue(adr('B1'))).toEqual(detailedErrorWithOrigin(ErrorType.NA, 'Sheet1!A1'))

We also add custom matchers for jest/jasmine, so the test can be written simply:

    expect(engine.getCellValue(adr('A1'))).toEqualError(detailedError(ErrorType.NA))
    expect(engine.getCellValue(adr('B1'))).toEqual(detailedError(ErrorType.NA))

An example of a usecase is to track the 'sources' of error in the spreadsheet:

  const vals = engine.getAllSheetsValues()

  for(let sheetName in vals) {
    const sheetVals = vals[sheetName]
    for(let x = 0; x<sheetVals.length; x++) {
      for(let y = 0; y<sheetVals[x].length; y++) {
        const val = sheetVals[x][y]
        if(val instanceof DetailedCellError) {
          const addr = {
            sheet: engine.getSheetId(sheetName)!,
            col: y,
            row: x
          }
          if (val.address === engine.simpleCellAddressToString(addr, -1)) {
            console.log(addr, val, engine.getCellFormula(addr))
          }
        }
      }
    }
  }

Custom matchers setup

In order to compare detailed errors without refactoring all unit tests, new matcher toEqualError was introduced. Defining new matcher for both jest and jasmine turned out to be tricky as there is type definition clash.

Current setup looks like this:

Jest matchers and additional boostrap file are defined in separate catalog to easily exclude them in jasmine tsconfig (tsconfig.test.json). Otherwise jasmine will complain during the tests.
For some reason using tsconfig.jest.json in main directory didn't do the job. Neither VSC nor IJ were able to detect new matchers correctly. Workaround was to move this file to ./test/tsconfig.json. It is crucial for local development and debugging as it overrides types definition to use jest ones.

This allows to run jest and jasmine tests and debug tests locally. Hopefully this will simplify someday.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature or improvement (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Additional language file or change to the existing one (translations)

Related issue(s):

Checklist:

My code follows the code style of this project,
My change requires a change to the documentation,
I described the modification in the CHANGELOG.md file.

wojciechczerniak

I have to request a review from @swistach for the tests configuration changes

wojciechczerniak · 2020-10-05T12:07:56Z

src/Cell.ts

@@ -112,6 +112,7 @@ export class CellError {
  constructor(
    public readonly type: ErrorType,
    public readonly message?: string,
+    public address?: SimpleCellAddress,


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

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.

wojciechczerniak · 2020-10-05T13:05:23Z

test/_setupFiles/jest/bootstrap.ts

+  // @ts-ignore
+  spyOn = jest.spyOn
+  expect.extend(toEqualError)
+})


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

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.

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

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

wojciechczerniak · 2020-10-05T13:08:15Z

test/tsconfig.json

+  "extends": "../tsconfig",
+  "include": ["../src", "../test"],
+  "exclude": []
+}


The file no longer has jest in its name. Looks like the main tsconfig for tests 🤔 If we're doing such changes they should be in a separate PR for easier discussion. I have to summon @swistach here for help 🧙

CC @swistach

See my comment above #529 (comment)

wojciechczerniak · 2020-10-05T13:24:47Z

src/interpreter/Interpreter.ts

    if (typeof val === 'number') {
      if (isNumberOverflow(val)) {
-        return new CellError(ErrorType.NUM, ErrorMessage.NaN)
+        val = new CellError(ErrorType.NUM, ErrorMessage.NaN)


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)

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

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?

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.

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.

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.

test/_setupFiles/jest/toEqualError.ts

wojciechczerniak · 2020-10-05T14:01:54Z

test/error-address-preservation.spec.ts

+    expect(engine.getCellValue(adr('A1'))).toEqualError(detailedErrorWithOrigin(ErrorType.NA, 'Sheet1!A1'))
+    expect(engine.getCellValue(adr('B1'))).toEqualError(detailedErrorWithOrigin(ErrorType.NA, 'Sheet1!A1'))
+  })
+})


Please add a few more test cases.

Does it work with Named Expressions (they are in -1 worksheet)?
Does it work with a function, nested function?
With operators?
Between two worksheets?
CRUD operations? (I guess that removing sheet/column will create a new #REF error anyway)

Added some tests. This logic is orthogonal to CRUDs, so any fault here would be fault in CRUDs, and should be tested with CRUDs.

Yeah, true 👍 Thanks for the tests. I've seen that we support all those scenarios, just wanted avoid regressions in the future.

test/matchers.spec.ts

src/CellValue.ts

wojciechczerniak · 2020-10-05T15:09:40Z

@swistach Nevermind! Let's talk it over

Co-authored-by: Wojciech Czerniak <wojciech.czerniak@gmail.com>

lgtm-com · 2020-10-05T22:21:06Z

This pull request fixes 8 alerts when merging 66b3b4d into 7152125 - view on LGTM.com

fixed alerts:

8 for Unused variable, import, function or class

lgtm-com · 2020-10-06T09:36:11Z

This pull request fixes 8 alerts when merging 8c4cff4 into 7152125 - view on LGTM.com

fixed alerts:

8 for Unused variable, import, function or class

izulin · 2020-10-08T09:26:13Z

@wojciechczerniak CellError address is now immutable.

Please see new tests for CYCLE resolving.

Also, namedExpression + Error address tests were using improper matcher...

lgtm-com · 2020-10-08T09:30:32Z

This pull request introduces 1 alert and fixes 8 when merging 1f112c6 into 7152125 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

fixed alerts:

8 for Unused variable, import, function or class

lgtm-com · 2020-10-08T09:42:51Z

This pull request introduces 1 alert and fixes 8 when merging e60f181 into 7152125 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

fixed alerts:

8 for Unused variable, import, function or class

lgtm-com · 2020-10-10T11:42:24Z

This pull request fixes 8 alerts when merging 0cbfd9e into c1630f0 - view on LGTM.com

fixed alerts:

8 for Unused variable, import, function or class

lgtm-com · 2020-10-10T12:02:34Z

This pull request fixes 8 alerts when merging 287ee6e into c1630f0 - view on LGTM.com

fixed alerts:

8 for Unused variable, import, function or class

codecov · 2020-10-10T12:15:29Z

Codecov Report

Merging #529 into develop will increase coverage by 0.00%.
The diff coverage is 93.75%.

@@           Coverage Diff            @@
##           develop     #529   +/-   ##
========================================
  Coverage    97.23%   97.23%           
========================================
  Files          155      156    +1     
  Lines        29429    29466   +37     
  Branches      3575     3583    +8     
========================================
+ Hits         28614    28651   +37     
  Misses         809      809           
  Partials         6        6

Impacted Files	Coverage Δ
src/Exporter.ts	`91.66% <91.66%> (ø)`
src/BuildEngineFactory.ts	`100.00% <100.00%> (ø)`
src/Cell.ts	`97.05% <100.00%> (+0.16%)`	⬆️
src/CellValue.ts	`100.00% <100.00%> (+8.26%)`	⬆️
src/Emitter.ts	`100.00% <100.00%> (ø)`
src/Evaluator.ts	`97.88% <100.00%> (ø)`
src/HyperFormula.ts	`99.38% <100.00%> (+<0.01%)`	⬆️
src/Serialization.ts	`100.00% <100.00%> (ø)`
src/index.ts	`100.00% <100.00%> (ø)`
src/interpreter/Interpreter.ts	`97.26% <100.00%> (+0.05%)`	⬆️
... and 5 more

wojciechczerniak

Thanks 🏆

error origins

4b7ed6f

izulin marked this pull request as draft September 23, 2020 11:51

Merge branch 'develop' into pu/errors

939ea86

izulin assigned izulin and voodoo11 Sep 23, 2020

izulin and others added 12 commits September 23, 2020 22:07

wrapper for errors

cac2039

Merge branch 'develop' into pu/errors

233c0f4

matchers

800d7a3

linter

9e3d560

Merge branch 'develop' into pu/errors

1103363

Move jest tsconfig to tests

b704304

Use error matcher where it is feasible

3df98ef

matchers fixed

594f748

extra test

357eaeb

Fix matchers, separate jest config

bb0841e

Linter

0ad6814

Merge branch 'develop' into pu/errors

8f4adcb

izulin marked this pull request as ready for review October 3, 2020 16:05

izulin requested a review from wojciechczerniak October 3, 2020 16:05

fix1

eef1888

wojciechczerniak suggested changes Oct 5, 2020

View reviewed changes

wojciechczerniak requested a review from mrpiotr-dev October 5, 2020 14:13

wojciechczerniak removed the request for review from mrpiotr-dev October 5, 2020 15:09

izulin and others added 6 commits October 5, 2020 22:17

Update test/_setupFiles/jest/toEqualError.ts

644644d

Co-authored-by: Wojciech Czerniak <wojciech.czerniak@gmail.com>

Update src/interpreter/Interpreter.ts

9b5b78f

Co-authored-by: Wojciech Czerniak <wojciech.czerniak@gmail.com>

refactor

11a1f68

.

5ca9ae0

refactor

9e6655e

.

86cbeca

izulin added 3 commits October 6, 2020 00:03

.

4118d64

linter

6352e4e

.

66b3b4d

tests

8c4cff4

CellError fields should be immutable. Fixed CYCLE address.

1f112c6

izulin requested a review from wojciechczerniak October 8, 2020 09:25

lgtm

e60f181

voodoo11 added 2 commits October 10, 2020 13:32

Some reminders

9edda38

Unused import

0cbfd9e

Merge branch 'develop' into pu/errors

287ee6e

wojciechczerniak approved these changes Oct 12, 2020

View reviewed changes

izulin merged commit 7fb1926 into develop Oct 12, 2020

izulin deleted the pu/errors branch October 12, 2020 17:54

bardek8 mentioned this pull request Nov 2, 2020

Dependencies are not correctly collected #549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

error origins #529

error origins #529

izulin commented Sep 23, 2020 •

edited by voodoo11

wojciechczerniak left a comment •

edited

wojciechczerniak Oct 5, 2020

izulin Oct 5, 2020

wojciechczerniak Oct 5, 2020

voodoo11 Oct 5, 2020 •

edited

voodoo11 Oct 5, 2020

voodoo11 Oct 10, 2020

wojciechczerniak Oct 5, 2020

voodoo11 Oct 5, 2020

wojciechczerniak Oct 5, 2020

izulin Oct 5, 2020

wojciechczerniak Oct 5, 2020

izulin Oct 5, 2020

wojciechczerniak Oct 6, 2020

izulin Oct 6, 2020

wojciechczerniak Oct 5, 2020

izulin Oct 6, 2020

wojciechczerniak Oct 6, 2020

wojciechczerniak commented Oct 5, 2020

lgtm-com bot commented Oct 5, 2020

lgtm-com bot commented Oct 6, 2020

izulin commented Oct 8, 2020

lgtm-com bot commented Oct 8, 2020

lgtm-com bot commented Oct 8, 2020

lgtm-com bot commented Oct 10, 2020

lgtm-com bot commented Oct 10, 2020

codecov bot commented Oct 10, 2020

wojciechczerniak left a comment

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

error origins #529

error origins #529

Conversation

izulin commented Sep 23, 2020 • edited by voodoo11

Context

How has this been tested?

Types of changes

Related issue(s):

Checklist:

wojciechczerniak left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voodoo11 Oct 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojciechczerniak commented Oct 5, 2020

lgtm-com bot commented Oct 5, 2020

lgtm-com bot commented Oct 6, 2020

izulin commented Oct 8, 2020

lgtm-com bot commented Oct 8, 2020

lgtm-com bot commented Oct 8, 2020

lgtm-com bot commented Oct 10, 2020

lgtm-com bot commented Oct 10, 2020

codecov bot commented Oct 10, 2020

Codecov Report

wojciechczerniak left a comment

Choose a reason for hiding this comment

izulin commented Sep 23, 2020 •

edited by voodoo11

wojciechczerniak left a comment •

edited

voodoo11 Oct 5, 2020 •

edited