kie-issues#1178: On the DMN Editor's Boxed Expression Editor, FEEL identifiers with spaces should be treated as non-breaking spaces. by danielzhe · Pull Request #2490 · apache/incubator-kie-tools

danielzhe · 2024-07-26T21:30:42Z

Closes: apache/incubator-kie-issues#1178

I found another edge case that is also fixed in this PR:

The other cases fixed are shown in the video below:
https://github.com/user-attachments/assets/608920b8-35c7-4724-9134-f61fe3a732b7

I also added more comments to the code, to make it easier to understand what is happening.

Sorry for the Portuguese in the pictures. Consider it as random chars to test the issues.

UPDATE: I'm attaching a file that shows the issues fixed if someone wants to use it to test it. About the other issue, reported in this comment here, you can test it with the "Loan Pre Qualification" example in Storybook or Online editor.
kie1178-english.dmn.txt

…entifiers with spaces should be treated as non-breaking spaces.

danielzhe · 2024-07-26T21:31:06Z

P.S.: we NEED to add test coverage to this feature asap.

danielzhe · 2024-07-26T21:40:11Z

packages/feel-input-component/src/FeelInput.tsx

              const text = model.getValue();
              const contentByLines = model.getLinesContent();
-              let startOfPreviousToken = 0;
+              let startOfPreviousVariable = 0;


This change was to make the variable name more precise.

danielzhe · 2024-07-28T21:56:04Z

I found a new issue, which will also be fixed with this PR, regarding line breaks. This issue happens only in some browsers and is related to the line ending (if it is *nix or Windows style). See in the video below a comparison between the broken behavior and the new one, @tiagobento

new.bug.mp4

The fix has not been pushed in the PR yet, it will come with the new unity tests.

…lines.

danielzhe · 2024-07-29T01:31:11Z

Changes applied! The issue about breaking lines, reported in the video above, it is also fixed.
I had to do some refactoring to make it easier to test and to organize the code. I'm testing only the SemanticTokensProvider that is what "translates" the parser result to the Monaco.

Let me know if the Jest tests are ok with our standards. I didn't found any case similar to this one in our codebase to compare. @tiagobento @ljmotta

tiagobento

@danielzhe Thanks! Love the comments and the changes you did. I'm a little extra picky when it comes to mocking, as I think we should only mock when absolutely necessary. If we could use the actual implementations instead it would be preferable! Thanks!

tiagobento · 2024-07-29T17:22:09Z

packages/feel-input-component/tests/semanticTokensProvider.test.ts

+ * limitations under the License.
+ */
+
+import { SemanticTokensProvider } from "../src/semanticTokensProvider";


Pls import it from @kie-tools/dmn-feel-antlr4-parser/dist/semanticTokensProvider instead of from src

But semanticTokensProvider is in this project, it is not from the parser. It is inside the feel-input-component, where this test is. Its work is to provide the semantic tokens to Monaco using what the parser dmn-feel-antlr4-parser did. 😄

Right, right. You can import it from itself then, to simulate usage as a 3rd party and avoid having to recompile the src code when executing tests, as tests run after compilation. You should be able to do @kie-tools/feel-input-component/dist/semanticTokensProvider

tiagobento · 2024-07-29T17:24:34Z

packages/feel-input-component/tests/semanticTokensProvider.test.ts

+  const tokensFoundByParser: FeelVariable[] = [];
+  const mockedParser = FeelVariablesParser as jest.Mocked<typeof FeelVariablesParser>;
+  jest
+    .spyOn(mockedParser.prototype, "parse")
+    .mockReturnValue({ availableSymbols: [], feelVariables: tokensFoundByParser });
+
+  const mockedFeelVariables: jest.Mocked<typeof FeelVariables> = FeelVariables as jest.Mocked<typeof FeelVariables>;
+  jest.spyOn(mockedFeelVariables.prototype, "parser", "get").mockReturnValue(mockedParser.prototype);
+
+  const cancellationTokenMock = {
+    isCancellationRequested: false,
+    onCancellationRequested: jest.fn().mockImplementation(),
+  };


Could we use the actual implementations? Doing so would make this test easier to read and remove the parsedTokens parameter need, no?

danielzhe · 2024-07-29T20:24:39Z

@tiagobento Changes applied! Now the test is testing all the stack, from feel-input-component to the dmn-feel-antlr4-parser.

tiagobento

Thanks! Please also integrate these new tests with the build:prod command... As they're currently not https://github.com/danielzhe/kie-tools/blob/feel-spaces/packages/feel-input-component/package.json

You can do

    "test": "rimraf dist-tests && run-script-if --ignore-errors \"$(build-env tests.ignoreFailures)\" --bool \"$(build-env tests.run)\" --then \"jest --silent --verbose --passWithNoTests\""

and add a && pnpm test at the end of the build:prod script.

danielzhe · 2024-07-30T01:48:42Z

@tiagobento New tests integrated!

ljmotta

@danielzhe I've made my review inline. The Apache header is the only comment that should be addressed, the others are optional as they're some observations regarding the test code.

ljmotta · 2024-07-30T01:58:41Z

packages/feel-input-component/tests/semanticTokensProvider.test.ts

+/*
+ * Copyright 2024 Red Hat, Inc. and/or its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */


Shouldn't it be an Apache header?

ouch
I did a copy-paste. Nice catch.

ljmotta · 2024-07-30T10:23:53Z

packages/feel-input-component/tests/semanticTokensProvider.test.ts

+  const cancellationTokenMock = {
+    isCancellationRequested: false,
+    onCancellationRequested: jest.fn().mockImplementation(),
+  };
+
+  const knownVariable =
+    "This is a variable with a very long name to reproduce the issue thousand one hundred and seventy-eight";
+
+  /**
+   * The 'parsedTokens' are the tokens that parser should found in the provided 'expression'.
+   * The 'expected' are the Monaco Semantic Tokens that we are expecting to pass to Monaco to paint it on the screen.
+   *
+   * Each Monaco Semantic Tokens is an array of 5 positions
+   * 0 = The start line of the token RELATIVE TO THE PREVIOUS LINE
+   * 1 = The start index of the token relative to the START of the previous token
+   * 2 = The length of the token
+   * 3 = The type of the token (GlobalVariable, Unknown, Function Parameter, etc.). It determines the color of the token
+   * 4 = Token modifier. It's always zero since we don't have this feature.
+   */
+  test.each([


Maybe this and tests should be inside another test.describe block? I mean, the variable is specific to a very long variable name.

Makes sense!

ljmotta · 2024-07-30T10:29:05Z

packages/feel-input-component/tests/semanticTokensProvider.test.ts

+function getDmnModel({
+  knownVariable,
+  expressionId,
+  expression,
+}: {
+  knownVariable: string;
+  expressionId: string;
+  expression: string;
+}) {


Overall I don't think the name and signature tells what this function do. It will get a DMN model with a context expression with one known entry (variable + expression). Also, you could add the return type to the function removing the necessity of creating a varible.

Do you have any suggestions for the name? For us, it doesn't really matter what is inside the DMN Model, since we just need a model with a setup to test the expression.

Also, about the inline, I didn't because of this: microsoft/TypeScript#241

We've been using this pattern in BEE, like here, for example.

I'll add the comment to make clear why it wasn't inlined.

Oh, I didn't know about this. Thanks for sharing! Regarding the name, just a few ideas:

getDmnModelWithContextExpression
getDmnModelWithContextEntry
generateDmnModelWithContextEntry

And some assignature examples:

getDmnModelWithContextEntry({ entry: { variable: string, expression: { value: string, id: string } } });

getDmnModelWithContextEntry({ entry: { variable: string, expression: string, expressionId: string, } });

ljmotta · 2024-07-30T10:32:13Z

packages/feel-input-component/tests/semanticTokensProvider.test.ts

+      cancellationTokenMock
+    );
+
+    const expectedSemanticMonacoTokens = expected.reduce((accumulator, value) => accumulator.concat(value), []);


You could use expected.flat() to archieve the same behavior.

danielzhe · 2024-07-31T13:13:30Z

@tiagobento Ready to merge.

kie-issues#1178: On the DMN Editor's Boxed Expression Editor, FEEL id…

873804d

…entifiers with spaces should be treated as non-breaking spaces.

danielzhe requested a review from tiagobento as a code owner July 26, 2024 21:30

danielzhe requested a review from ljmotta July 26, 2024 21:31

danielzhe commented Jul 26, 2024

View reviewed changes

tiagobento added the area:dmn label Jul 26, 2024

danielzhe added 2 commits July 28, 2024 22:22

Added first tests and fixed a new issue found relative with breaking …

9f2c986

…lines.

Lock file

396fe01

tiagobento reviewed Jul 29, 2024

View reviewed changes

Test without mocks.

b4f50e6

tiagobento requested changes Jul 29, 2024

View reviewed changes

Run tests script

0ae1824

ljmotta reviewed Jul 30, 2024

View reviewed changes

Fixed header. Nested test decribe. Fixed import.

4703652

tiagobento approved these changes Jul 30, 2024

View reviewed changes

Renaming getDmnModel function and parameters.

df6ce71

ljmotta approved these changes Jul 31, 2024

View reviewed changes

tiagobento merged commit eb0045d into apache:main Jul 31, 2024

Conversation

danielzhe commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielzhe commented Jul 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielzhe commented Jul 28, 2024

Uh oh!

danielzhe commented Jul 29, 2024

Uh oh!

tiagobento left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielzhe commented Jul 29, 2024

Uh oh!

tiagobento left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielzhe commented Jul 30, 2024

Uh oh!

ljmotta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielzhe commented Jul 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danielzhe commented Jul 26, 2024 •

edited

Loading

tiagobento left a comment •

edited

Loading