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

Tabular extension exec tests #458

Merged
merged 27 commits into from
Dec 7, 2023

Conversation

f3l1x98
Copy link
Contributor

@f3l1x98 f3l1x98 commented Oct 30, 2023

Closes #417

This PR adds tests to all the block-executors of the tabular extension.

Note: This conflicts with #456 (this is newer -> the other will have to be adjusted)

Important changes

  • Moved the file-util.ts from libs/extensions/std to libs/execution due to required usage in tabular extension tests.
  • Extracted libs/extensions/std/exec/src/text-file-interpreter-executor.ts#splitLines into lib/execution/src/lib/util/string-util.ts also due to usage in tabular tests
  • Refactored libs/execution/test/utils.ts into folder with multiple utils (old utils.ts renamed to utils/test-infrastructure-util.ts)
  • Added jsdom fixes for structuredClone and setImmediate

@f3l1x98
Copy link
Contributor Author

f3l1x98 commented Oct 30, 2023

I have found two things that need clarification:

  1. const indexOfMatchingHeader = headerRow.findIndex(
    (headerColumnName) => headerColumnName === columnDefinition.name,
    );

    Is this case-sensitive on purpose?
  2. If the TableInterpreterExecutor is called for a Sheet with data (for example 8 rows of data) but with columnDefinitions: []; this will result into a table that satisfies the following:
      // [...]
      const result = await parseAndExecuteExecutor(
        text,
        testWorkbook.getSheetByName('Sheet1') as R.Sheet,
      );
      // [...]
      const table = result.right;
      expect(table.getNumberOfColumns()).toEqual(0);
      expect(table.getNumberOfRows()).toEqual(8);

This is due to Table#addRow only verifying whether rowLength === this.columns.size, however that is always true in this case because TableInterpreterExecutor#constructAndValidateTableRow returns an empty TableRow if there are no columns. Due to that Table#addRow will be executed without any issue and increment the row count, however there is no data in the table.
TLDR: Table indicates there are rows stored, even though there are none (there should not be any).

@f3l1x98 f3l1x98 requested a review from georg-schwarz October 30, 2023 11:43
@georg-schwarz
Copy link
Member

  1. const indexOfMatchingHeader = headerRow.findIndex(
    (headerColumnName) => headerColumnName === columnDefinition.name,
    );

    Is this case-sensitive on purpose?

Yes, I believe so.

  1. If the TableInterpreterExecutor is called for a Sheet with data (for example 8 rows of data) but with columnDefinitions: []; this will result into a table that satisfies the following:
      // [...]
      const result = await parseAndExecuteExecutor(
        text,
        testWorkbook.getSheetByName('Sheet1') as R.Sheet,
      );
      // [...]
      const table = result.right;
      expect(table.getNumberOfColumns()).toEqual(0);
      expect(table.getNumberOfRows()).toEqual(8);

This is due to Table#addRow only verifying whether rowLength === this.columns.size, however that is always true in this case because TableInterpreterExecutor#constructAndValidateTableRow returns an empty TableRow if there are no columns. Due to that Table#addRow will be executed without any issue and increment the row count, however there is no data in the table. TLDR: Table indicates there are rows stored, even though there are none (there should not be any).

Good catch! I'd expect that only a row is added if there is data in it.
It might only make sense when you add all empty rows, then add content to the rows by adding columns. But I don't think we use this anywhere.

@f3l1x98
Copy link
Contributor Author

f3l1x98 commented Nov 6, 2023

Good catch! I'd expect that only a row is added if there is data in it.
It might only make sense when you add all empty rows, then add content to the rows by adding columns. But I don't think we use this anywhere.

We decided that this fix is ok, we just have to document this behavior somewhere.

@georg-schwarz georg-schwarz merged commit 2e5f611 into main Dec 7, 2023
2 checks passed
@georg-schwarz georg-schwarz deleted the feature/tabular-extension-exec-tests branch December 7, 2023 12:25
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] tabular extension exec tests
2 participants