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

Input to ServiceTestAssertLambda is updated to regular string #629

Merged
merged 7 commits into from
Nov 2, 2021

Conversation

mrudula-gs
Copy link
Contributor

@mrudula-gs mrudula-gs commented Oct 29, 2021

Summary

Closes #586

Due to discrepancies in the test runners for mapping and service, we have don't need to do any (un)escaping for service test assertion data like what we do for mapping test assertion data. For better context:
See #586
See finos/legend-engine#429

How did you test this change?

Check with the test case in the linked issue

@changeset-bot
Copy link

changeset-bot bot commented Oct 29, 2021

🦋 Changeset detected

Latest commit: c220265

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages
Name Type
@finos/legend-studio Patch
@finos/legend-extension-dsl-data-space Patch
@finos/legend-extension-dsl-diagram Patch
@finos/legend-extension-dsl-serializer Patch
@finos/legend-extension-dsl-text Patch
@finos/legend-extension-external-store-service Patch
@finos/legend-studio-app Patch
@finos/legend-studio-extension-management-toolkit Patch
@finos/legend-studio-extension-query-builder Patch
@finos/legend-manual-tests Patch
@finos/legend-query-app Patch
@finos/legend-studio-deployment Patch
@finos/legend-query-deployment Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@finos-cla-bot finos-cla-bot bot added the cla-present CLA Signed label Oct 29, 2021
@finos-cla-bot
Copy link

finos-cla-bot bot commented Nov 1, 2021

Thank you for your contribution and Welcome to our Open Source Community!

To make sure your pull request is accepted successfully, we ask all our open source contributors to sign a Contributor License Agreement; having reviewed our contributor list, we require a CLA for the following people : (@anumalamrudula).

In order to sign a CLA with FINOS, just submit a Pull Request with a simple change to this file (adding an empty line, or some random text at the bottom); this will trigger the EasyCLA bot, which will post a comment to the Pull Request stating whether all PR contributors are covered by FINOS CLA; if not covered, the bot will post instructions on how to sign the CLA.

Thanks once again for your contribution. Let us work with you to make the CLA process quick, easy and efficient so we can move forward with reviewing and accepting your pull request. Feel free to email help@finos.org for any questions.

cc @maoo @agitana @mcleo-d

@finos-cla-bot finos-cla-bot bot removed the cla-present CLA Signed label Nov 1, 2021
@akphi
Copy link
Contributor

akphi commented Nov 1, 2021

@mrudula-gs Did we file a bug for this? We should link it in the PR description and the changeset.

@finos-cla-bot
Copy link

finos-cla-bot bot commented Nov 1, 2021

Thank you for your contribution and Welcome to our Open Source Community!

To make sure your pull request is accepted successfully, we ask all our open source contributors to sign a Contributor License Agreement; having reviewed our contributor list, we require a CLA for the following people : (@anumalamrudula).

In order to sign a CLA with FINOS, just submit a Pull Request with a simple change to this file (adding an empty line, or some random text at the bottom); this will trigger the EasyCLA bot, which will post a comment to the Pull Request stating whether all PR contributors are covered by FINOS CLA; if not covered, the bot will post instructions on how to sign the CLA.

Thanks once again for your contribution. Let us work with you to make the CLA process quick, easy and efficient so we can move forward with reviewing and accepting your pull request. Feel free to email help@finos.org for any questions.

cc @maoo @agitana @mcleo-d

@akphi
Copy link
Contributor

akphi commented Nov 1, 2021

@mrudula-gs 2 things

  1. Could you properly link your PR to the issue? - See https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
  2. For the 2 places that we make modifications, let's use the following comment as clarification
// NOTE: due to discrepancies in the test runners for mapping and service, we have don't need 
// to do any (un)escaping here like what we do for mapping test assertion data. For better context:
// See https://github.com/finos/legend-studio/issues/586 
// See https://github.com/finos/legend-engine/issues/429

Thanks!

@finos-cla-bot
Copy link

finos-cla-bot bot commented Nov 2, 2021

Thank you for your contribution and Welcome to our Open Source Community!

To make sure your pull request is accepted successfully, we ask all our open source contributors to sign a Contributor License Agreement; having reviewed our contributor list, we require a CLA for the following people : (@anumalamrudula).

In order to sign a CLA with FINOS, just submit a Pull Request with a simple change to this file (adding an empty line, or some random text at the bottom); this will trigger the EasyCLA bot, which will post a comment to the Pull Request stating whether all PR contributors are covered by FINOS CLA; if not covered, the bot will post instructions on how to sign the CLA.

Thanks once again for your contribution. Let us work with you to make the CLA process quick, easy and efficient so we can move forward with reviewing and accepting your pull request. Feel free to email help@finos.org for any questions.

cc @maoo @agitana @mcleo-d

@finos-cla-bot finos-cla-bot bot removed the cla-present CLA Signed label Nov 2, 2021
@finos-cla-bot finos-cla-bot bot added the cla-present CLA Signed label Nov 2, 2021
@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #629 (c220265) into master (f8314e8) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #629      +/-   ##
==========================================
- Coverage   44.37%   44.35%   -0.02%     
==========================================
  Files         891      892       +1     
  Lines       39380    39402      +22     
  Branches     8995     9005      +10     
==========================================
+ Hits        17473    17476       +3     
- Misses      21848    21867      +19     
  Partials       59       59              
Impacted Files Coverage Δ
...e/element-editor-state/service/ServiceTestState.ts 23.83% <0.00%> (-0.14%) ⬇️
.../legend-application/src/components/ActionAlert.tsx 9.30% <0.00%> (-1.81%) ⬇️
...ackages/legend-shared/src/format/FormatterUtils.ts 89.36% <0.00%> (-1.75%) ⬇️
...ation/pureGraph/to/helpers/V1_ProcessingContext.ts 28.88% <0.00%> (-1.35%) ⬇️
...sformation/pureGraph/from/V1_MappingTransformer.ts 81.01% <0.00%> (-0.77%) ⬇️
...re/packageableElements/diagram/RelationshipView.ts 73.86% <0.00%> (-0.56%) ⬇️
.../legend-application/src/stores/ApplicationStore.ts 18.88% <0.00%> (-0.47%) ⬇️
...legend-query/src/stores/QueryBuilderFilterState.ts 50.15% <0.00%> (-0.46%) ⬇️
...egend-shared/src/application/SerializationUtils.ts 14.28% <0.00%> (-0.35%) ⬇️
...nents/editor/edit-panel/uml-editor/ClassEditor.tsx 55.81% <0.00%> (-0.27%) ⬇️
... and 38 more

@akphi
Copy link
Contributor

akphi commented Nov 2, 2021

@mrudula-gs Sorry, took the liberty myself to update the description and the changeset for a clearer explanation since this problem is quite confusing. I would also note that this is just the temporary solution on Studio side for now until we have a more definitive strategy for finos/legend-engine#429 as well as when @gs-gunjan and @MauricioUyaguari refactored the mapping test flow to use engine mapping test runner, we would need to re-evaluate this again.

@akphi akphi merged commit 2d855dc into finos:master Nov 2, 2021
@github-actions github-actions bot mentioned this pull request Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-present CLA Signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Escape character is not correctly handled in Service Test
3 participants