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

New Check: Activate fixed point arithmetic #667

Merged
merged 11 commits into from Feb 21, 2019

Conversation

Projects
None yet
2 participants
@pcf0
Copy link
Contributor

pcf0 commented Feb 16, 2019

It's still work in process, because:

  • I don't know how to write proper unit tests for this check because of the use of cl_abap_compiler
  • string templates can't be detected because get_full_name_for_position of cl_abap_compiler returns an exception

This check runs, if fixed point arithmetic is not active.
If you can activate it safely, it will return the configured error type.
If you can't activate it safely, it will return info messages.

I used this program to test this check (includes results of check):
https://gist.github.com/pcf0/d184e57c1f4d0aa64f9217446bd880c5

closes #665

pcf0 added some commits Feb 16, 2019

@pcf0

This comment has been minimized.

Copy link
Contributor Author

pcf0 commented Feb 16, 2019

I don't get the message Parser error(Unknown statement), ABAP version v702 (3aa8ef2#annotation_14045901)
Because judging by this source, cl_abap_compiler->get_full_name_for_position exists since 2001: https://www.sapdatasheet.org/abap/clas/cl_abap_compiler.html#seocompo-get_full_name_for_position

@larshp

This comment has been minimized.

Copy link
Owner

larshp commented Feb 17, 2019

its the part with CONV i( <ls_token>-col ) that does not work on old versions

@pcf0

This comment has been minimized.

Copy link
Contributor Author

pcf0 commented Feb 17, 2019

oh thanks, i didn't notice 😊

@larshp

This comment has been minimized.

Copy link
Owner

larshp commented Feb 17, 2019

and pretty print the full class source

@larshp

This comment has been minimized.

Copy link
Owner

larshp commented Feb 17, 2019

there are several other checks which uses cl_abap_compiler and also does not have unit tests

pcf0 added some commits Feb 17, 2019

@pcf0

This comment has been minimized.

Copy link
Contributor Author

pcf0 commented Feb 17, 2019

Overnight, I had an idea on how to do unit testing. What do you think about? eb6c73a

  1. generate a temporary report with an unused name
  2. run unit test
  3. delete the report
@larshp

This comment has been minimized.

Copy link
Owner

larshp commented Feb 17, 2019

yeah, its the right way to do it, however there is currently nothing like this in abapOpenChecks

I dont want to risk running this in a customer system

abapGit has a global flag to enable dangerous tests(which does changes to the customer system), but currently abapOpenChecks does not have any persistency, so that also has to be done somehow
image

@larshp

This comment has been minimized.

Copy link
Owner

larshp commented Feb 17, 2019

plus it might be an idea to store each test case in a specific repository, much like https://github.com/abapGit-tests

Its a lot of work to put a complex setup as text in unit tests

@pcf0

This comment has been minimized.

Copy link
Contributor Author

pcf0 commented Feb 17, 2019

Wow, you're fast, I did not realize you already answered.
While I fixed the ababLint message and some errors, I also thought it would be better to set the risk level to DANGEROUS. So i splitted the tests.

Im not so deep into abapGit, so i dont get your statement

abapGit has a global flag to enable dangerous tests(which does changes to the customer system), but currently abapOpenChecks does not have any persistency, so that also has to be done somehow

How would you like to proceed?
Should I delete or leave these dangerous tests?

Also abapLint outputs a parser_error https://github.com/larshp/abapOpenChecks/pull/667/files#annotation_14070923
Is it because of the FIXED-POINT ARITHMETIC?
According to this documentation it exists in 702: https://web.archive.org/web/20130817151112/http://help.sap.com/abapdocu_70/en/ABAPINSERT_REPORT.htm

@larshp

This comment has been minimized.

Copy link
Owner

larshp commented Feb 17, 2019

abaplint: its a bug, it does not know FIXED-POINT, opened larshp/abaplint#364
see https://syntax.abaplint.org/?filter=insert#/statement/InsertReport

Please delete the dangerous tests, it should not be possible to accidentally change/create artifacts in the customer system. The customer dev system might have dangerous tests enabled

pcf0 added some commits Feb 17, 2019

Revert "Fixed tests"
This reverts commit 4aad058.
Revert "unit tests"
This reverts commit eb6c73a.

pcf0 added some commits Feb 18, 2019

@pcf0

This comment has been minimized.

Copy link
Contributor Author

pcf0 commented Feb 20, 2019

Today I tested this check at work on an very old print program and found 2 errors:

  • there was an short dump if you assign a new class to a variable
  • when you assign an attribute from a class, it only recognizes the class and not the attribute

These are fixed with 36055d6

@larshp

This comment has been minimized.

Copy link
Owner

larshp commented Feb 21, 2019

cool, I havent tested the code yet, but looks good overall

@larshp larshp merged commit 6c77e68 into larshp:master Feb 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.