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

run more unit tests via transpiler #153

Merged
merged 46 commits into from
Aug 2, 2022
Merged

run more unit tests via transpiler #153

merged 46 commits into from
Aug 2, 2022

Conversation

larshp
Copy link
Collaborator

@larshp larshp commented Jul 21, 2022

cl_abap_unit_assert=>assert_equals( exp = zif_aff_writer=>type_info-string act = cut->get_json_type_from_description( get_element_description( VALUE zif_aff_writer=>enum_type_info( ) ) ) ).

@larshp larshp changed the title wip run more unit tests via transpiler Jul 29, 2022
@larshp larshp marked this pull request as ready for review July 29, 2022 15:43
@larshp
Copy link
Collaborator Author

larshp commented Jul 29, 2022

this runs a bunch more unit tests via transpiler, ready for review/merge

one ABAP related change, see comment above

@schneidermic0
Copy link
Contributor

If we want to get rid of enum support, because AFF types shall be compatible with release 7.02, then I suggest to remove also (at least) the tested production code; OR cl_abap_typedescr=>typekind_enum:

cl_abap_typedescr=>typekind_hex OR cl_abap_typedescr=>typekind_num OR cl_abap_typedescr=>typekind_enum.

@larshp
Copy link
Collaborator Author

larshp commented Aug 1, 2022

^can we defer that to #65 ?

its kind of a pity removing something if it works. The AFF might not be the only future use-case for this tool, so 🤷‍♂️

@schneidermic0
Copy link
Contributor

I am happy if we keep the functionality, but then it would be good that it is also tested, isn't it?

I understand you removed it because the transpiler hasn't supported it yet, right? If so, wouldn't it make sense to move this single assert in a specific test method and set the test to ignore? Then we can at least run it in an ABAP system.

@larshp
Copy link
Collaborator Author

larshp commented Aug 2, 2022

I am happy if we keep the functionality, but then it would be good that it is also tested, isn't it?

I understand you removed it because the transpiler hasn't supported it yet, right? If so, wouldn't it make sense to move this single assert in a specific test method and set the test to ignore? Then we can at least run it in an ABAP system.

yea, good idea

@larshp
Copy link
Collaborator Author

larshp commented Aug 2, 2022

@schneidermic0 added in above commit

Copy link
Contributor

@schneidermic0 schneidermic0 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just a minor question.

abap_transpile.json Outdated Show resolved Hide resolved
Co-authored-by: Michael Schneider <[email protected]>
@schneidermic0 schneidermic0 merged commit cd44700 into main Aug 2, 2022
@schneidermic0 schneidermic0 deleted the hvam/upd2107 branch August 2, 2022 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants