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.

Sign up for GitHub

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

Jump to bottom

test: refactor JsonParser unit tests and type handling enhancements #2801

Merged
merged 6 commits into from
Aug 22, 2023
Merged

test: refactor JsonParser unit tests and type handling enhancements #2801

merged 6 commits into from
Aug 22, 2023

Conversation

csouchet
Copy link
Member

This pull request makes three improvements to JsonParser and its associated unit tests:

  • Enforce the BpmnJsonModel type for the JsonParser dataset in the unit tests: This is a first step in addressing issue [BUG] Extra parsing of lane elements which are not in the BPMN spec #2188.
    This enforcement ensures that the JSON passed to the parser adheres to the required format.
    ℹ️ It's worth noting that I have observed cases where TypeScript failed to detect incorrect JSON formats when the type was not explicitly set.

  • Simplified JSON data initialization for JsonParser unit tests: This makes our test code more readable and easier to maintain.

  • Detection of the TypeScript-based default property of TProcess in the ProcessConverter.

These improvements enhance code reliability, readability and accuracy.

@csouchet csouchet added the chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...) label Aug 21, 2023
@csouchet csouchet changed the title chore(unit tests): Refactoring JsonParser unit tests and type handling enhancements chore(unit tests): refactoring JsonParser unit tests and type handling enhancements Aug 21, 2023
@github-actions
Copy link

github-actions bot commented Aug 21, 2023 •
edited
Loading

♻️ PR Preview 25e0248 has been successfully destroyed since this PR has been closed.

🤖 By surge-preview

@github-actions
Copy link

github-actions bot commented Aug 21, 2023 •
edited
Loading

♻️ PR Preview 25e0248 has been successfully destroyed since this PR has been closed.

🤖 By surge-preview

csouchet
csouchet commented Aug 21, 2023
View reviewed changes
test/unit/component/parser/json/BpmnJsonParser.sequenceFlow.default.test.ts
@@ -82,11 +82,14 @@ describe('parse bpmn as json for default sequence flow', () => {
});

it(`should NOT convert, when an sequence flow (defined as default) is an attribute of 'process' and attached to a flow node where is NOT possible in BPMN Semantic`, () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

thought: I kept this test; however, given TypeScript's restriction that prevents accessing the default property on an object if it's not defined in the type, I'm not sure it's necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Remember that the BpmnJsonModel instance is created from an XML source in real live and no types check is done at runtime. So, at runtime, we can actually have to deal with such data.

@csouchet csouchet force-pushed the enforce_the_BpmnJsonModel_type_in_the_unit_tests_of_the_JsonParser branch 2 times, most recently from 4fc7de9 to dd10083 Compare August 21, 2023 14:30
@tbouffard tbouffard changed the title chore(unit tests): refactoring JsonParser unit tests and type handling enhancements test: refactor JsonParser unit tests and type handling enhancements Aug 21, 2023
@csouchet csouchet marked this pull request as ready for review August 21, 2023 14:45
@csouchet csouchet requested a review from tbouffard August 21, 2023 14:45
@csouchet csouchet force-pushed the enforce_the_BpmnJsonModel_type_in_the_unit_tests_of_the_JsonParser branch from dd10083 to 329e1cf Compare August 21, 2023 16:21
tbouffard
tbouffard reviewed Aug 21, 2023
View reviewed changes
Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

LGTM. This is now much more readable and types checks are better enforced 🏅

I suggest to update some comments, otherwise, some valuable information may be missed and wrong conclusions would be included IMHO.

src/component/parser/json/converter/ProcessConverter.ts
const defaultFlow = bpmnElement.default;
if (ShapeUtil.isWithDefaultSequenceFlow(kind) && defaultFlow) {
this.defaultSequenceFlowIds.push(defaultFlow);
if ('default' in bpmnElement && ShapeUtil.isWithDefaultSequenceFlow(kind)) {
Copy link
Member

Choose a reason for hiding this comment

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

praise: Nice!

test/unit/component/parser/json/BpmnJsonParser.flowNode.test.ts Outdated Show resolved Hide resolved
test/unit/component/parser/json/BpmnJsonParser.sequenceFlow.conditional.test.ts Outdated Show resolved Hide resolved
test/unit/component/parser/json/BpmnJsonParser.sequenceFlow.default.test.ts
@@ -82,11 +82,14 @@ describe('parse bpmn as json for default sequence flow', () => {
});

it(`should NOT convert, when an sequence flow (defined as default) is an attribute of 'process' and attached to a flow node where is NOT possible in BPMN Semantic`, () => {
Copy link
Member

Choose a reason for hiding this comment

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

Remember that the BpmnJsonModel instance is created from an XML source in real live and no types check is done at runtime. So, at runtime, we can actually have to deal with such data.

test/unit/component/parser/json/BpmnJsonParser.sequenceFlow.default.test.ts Outdated
definitions: {
targetNamespace: '',
process: {
id: 'Process_1',
// To enforce the type and test a case who never should happen
Copy link
Member

Choose a reason for hiding this comment

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

praise: see my previous comment, this cannot happen if the BpmnJsonModel was always created via some code. However, the actual data are generated from the BPMN source and so, such this use case can happen.

@csouchet csouchet force-pushed the enforce_the_BpmnJsonModel_type_in_the_unit_tests_of_the_JsonParser branch from 329e1cf to 3b3438d Compare August 22, 2023 11:57
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@csouchet csouchet requested a review from tbouffard August 22, 2023 12:33
tbouffard
tbouffard approved these changes Aug 22, 2023
View reviewed changes
Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

LGTM

@csouchet csouchet merged commit 9a01996 into master Aug 22, 2023
@csouchet csouchet deleted the enforce_the_BpmnJsonModel_type_in_the_unit_tests_of_the_JsonParser branch August 22, 2023 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbouffard tbouffard tbouffard approved these changes

Assignees
No one assigned
Labels
chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@csouchet @tbouffard