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

288 Enable COBOL Check to issue warnings/errors when a call statement is not mocked. #316

Conversation

AkashKumar7902
Copy link
Contributor

Fixes: #288

Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
@AkashKumar7902 AkashKumar7902 marked this pull request as draft August 30, 2023 16:33
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
@AkashKumar7902
Copy link
Contributor Author

AkashKumar7902 commented Aug 31, 2023

@Rune-Christensen
Hey
In the testconfig.properties file, what value should I set for cobolcheck.unmock.call.alerttype.error ?

Copy link
Collaborator

@Rune-Christensen Rune-Christensen left a comment

Choose a reason for hiding this comment

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

Hi @AkashKumar7902
There are a few issues, and of course, I would like tests to verify the functionality you have implemented.

First, after sleeping on the issue, I am not sure this is the right approach.
I am in doubt wether or not we are being too stringent with this feature, and that we should allow a version that does not raise a warning or error during COBOL Check.
The issue is, that there could be three calls in a program, but your unit test only touches one of them, then why would we raise a warning or error on the ones that are not in scope of the test.
Maybe this should be an error/warning during runtime of the COBOL program, instead of a warning/error from COBOL Check.
I am going to discuss this with colleagues, before making a decision.

Please wait for our decision on this, because it greatly affects the implementation.

Second, I should tell you about our undocumented design pattern of COBOL Check.
We have three tiers of programs. Workers, features and services.
Workers are the top level programs, that implements and manages the top level flow of the programs. Workers use features and services to perform this flow management.
Features provide, well features, to the workers, these should be isolated, and not need to call other features. (I know there are exceptions to this, but we try to not implement more of those) Features use services to implement the functionality that they expose to the workers.
Services is the bottom level programs, they provide the basic functionality to features, especially functionality that several features are using. Services are preferred to be isolated, but may sometimes use other services, and rarely use features.

Your code looks good, but please refaktor it, so workers use features, and features use services.

Thank you

@@ -32,6 +32,7 @@ ERR029 = ERR029: NumericFields.setDataTypeOf() was called with null dataType.
ERR030 = ERR030: Command line missing program argument '-p programName' .
ERR031 = ERR031: A test suite with the name %1$s already exists.
ERR032 = ERR032: A test case with the name %1$s already exists in the test suite %2$s.
ERR033 = ERR033: Call Statement in Line %1$s of the source code is not mocked in %2$s of testSuite %3$s.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change to "not mocked in testcase "%2$s" " to indicate that the first value is the testcase.
And also put quotes around the testsuite name.
Thank you

Copy link
Contributor Author

@AkashKumar7902 AkashKumar7902 Sep 1, 2023

Choose a reason for hiding this comment

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

@Rune-Christensen
Hey,
Sorry for not adding quotes.
I will be adding it in my new commit.

https://github.com/openmainframeproject/cobol-check/pull/316/files#diff-9d10e676c4992827a0cac77d70a1d1b995c80694ad957793df629043a5121121R154
I believe the above line is solving the "testcase" naming issue.
Thank You

@@ -41,6 +42,7 @@ WRN005 = WRN005: Test results input file not found: %1$s. Define in config as te
WRN006 = WRN006: IOException writing test results to file: %1$s in ProcessOutputWriter.writeProcessOutputToFile(...)
WRN007 = WRN007: IOException reading test results from current process
WRN008 = WRN008: Access denied: Could not change permissions for %1$s
WRN009 = WRN009: Call Statement in Line %1$s of the source code is not mocked in %2$s of testSuite %3$s.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above, thank you

@@ -1,15 +1,18 @@
package org.openmainframeproject.cobolcheck.features.testSuiteParser;

import org.openmainframeproject.cobolcheck.exceptions.*;
import org.openmainframeproject.cobolcheck.features.interpreter.StringTokenizerExtractor;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am going to ask for some rewriting here.
TestSuiteParser is a feature in the structure of COBOL Check.
As general rule, features should not be importing features.
I know there are exceptions to that, but it is not the case for TestSuiteParser.
You should be able to get the values you are looking for, from a services, instead of a feature.
Thank you

@@ -7,11 +7,15 @@
import org.openmainframeproject.cobolcheck.services.Messages;
import org.openmainframeproject.cobolcheck.services.StringHelper;
import org.openmainframeproject.cobolcheck.services.cobolLogic.NumericFields;
import org.openmainframeproject.cobolcheck.workers.Generator;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am going to ask for some rewriting here.
TestSuiteParserController is a feature in the structure of COBOL Check.
As a rule, features should not be importing workers.
You should be able to get the values you are looking for, from a services, instead of a worker.
Or maybe the functionality that you want to implement, should be located in a worker, instead of a feature.
Thank you

@AkashKumar7902
Copy link
Contributor Author

AkashKumar7902 commented Sep 1, 2023

@Rune-Christensen
I am currently asserting the error messages, if you want me to assert warning messages as well I would like to know which logging library would be best.
Thank you

Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
@AkashKumar7902 AkashKumar7902 marked this pull request as ready for review September 1, 2023 09:32
@Rune-Christensen
Copy link
Collaborator

Hi @AkashKumar7902
We discussed this issue. And have decided that this is the wrong approach. Sorry for not figuring this out sooner.
The work you have done is great, and the tests are spot on, thank you.

COBOL Check should not be issueing warnings or errors for unmocked call statements, while it is processing the unit test and COBOL source. This could result in confusing the end user, because of a requirement to mock every call, even though the call is not used in the unit test.

Instead COBOL Check should add code to the resulting COBOL code that will cause the unit tests to fail, when the user is trying to run a mocked call statement.

It should be possible to enable and disable this, using a configuration variable, that is set to default true. (true means create the error)

Examples:
A call that is not mocked:

100-Demo section.
Call 'dummy'
.

Results in:

100-Demo section.
* Call 'dummy'
Add 1 to number-of-errors
display "Call not mocked in testcase "testcasename" in testsuite "testsuitename" 
display "All used calls should be mocked, to ensure the unit test has control over input data"

The same call that is mocked results in:

100-Demo section.
* Call 'dummy'
Evaluate testsuite and testcase
When "some-testsuite" and "some-testcase"
     continue
When other
    perform when-other-section
end-evaluate
.
when-other-section section.

* Call 'dummy'
Add 1 to number-of-errors
display "Call not mocked in testcase "testcasename" in testsuite "testsuitename" 
display "All used calls should be mocked, to ensure the unit test has control over input data"
.

Again, sorry for not figuring this out sooner.
I hope you are not discouraged from trying to solve the issue this way.
Thank you

@AkashKumar7902
Copy link
Contributor Author

@Rune-Christensen
In the example you provided, what role will number-of-errors play ?

@Rune-Christensen
Copy link
Collaborator

@AkashKumar7902

@Rune-Christensen In the example you provided, what role will number-of-errors play ?

It is the variable that has the number of failed tests when the generated COBOL code is running, i believe the actual variable is called ==UT==NUMBER-FAILED

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.

Enable COBOL Check to issue warnings/errors when a call statement is not mocked.
2 participants