FWD » Core Development » Base Language

Vladimir Tsichevski wrote:

Another issue discovered: the R-INDEX implementation does not support CHARACTER case sensitivity, the comparison is always case-sensitive now. Will fix this as well.

This fixed in rev. 15572.

Please coordinate with Marian to check these into the Testcases project.

Review of 8309a/rev. 15573:

• I am OK with the functionality of the changes and the overall refactoring!
• minor formatting issue: (int) i instead of (int)i when casting from double. PS: I always thought this is the coding standard, but I may have recalled incorrectly - please let me know if this was out of place.
• integer lastIndexOf(Text src, String target, NumberType i) creates src1 and target1 - is this required? Can't src1 and target1 be inlined?
• integer lastIndexOf(final String src, final String target, final NumberType i) can be also rewritten to use ternary ?: operator.
• there are many places where final was added to the method parameters. This remembered me of the Overuse-of-final-in-Variables . I am not a coding standards author, so I can't tell properly if the changes are honoring the statement there ... maybe Greg can provide a better insight, as he is one of the authors of that document.
• The indexOfChecked should have its parameters aligned according to Breaking Long Lines
• In indexOfChecked, strSrc = txtSrc.getValue(); can be moved after the isUnknown check - this avoids an extra call to getValue if not needed.

• there are many places where final was added to the method parameters. This remembered me of the Overuse-of-final-in-Variables . I am not a coding standards author, so I can't tell properly if the changes are honoring the statement there ... maybe Greg can provide a better insight, as he is one of the authors of that document.

My be you are right. Explicitly marking a variable as final makes me sure it is really final and makes it easier to understand the code I was not the author of.
BTW I have not found any "Overuse-of-final-in-Variables" section in the document.

The correct location is Overruse of final in Variables.

Alexandru is correct. Please do not make these changes to add final everywhere.

Greg Shah wrote:

• there are many places where final was added to the method parameters. This remembered me of the Overuse-of-final-in-Variables . I am not a coding standards author, so I can't tell properly if the changes are honoring the statement there ... maybe Greg can provide a better insight, as he is one of the authors of that document.

My be you are right. Explicitly marking a variable as final makes me sure it is really final and makes it easier to understand the code I was not the author of.
BTW I have not found any "Overuse-of-final-in-Variables" section in the document.

The correct location is Overruse of final in Variables.

Alexandru is correct. Please do not make these changes to add final everywhere.

All final modifiers removed in all method parameters in rev. 15580.

When updating I get:

Tree is up to date at revision 15573 of branch bzr+ssh://localhost:2224/opt/secure/code/p2j_repo/p2j/active/8309a

The revisions you are mentioning look newer; maybe you rebased, but not pushed?

Alexandru Lungu wrote:

When updating I get:
[...]

The revisions you are mentioning look newer; maybe you rebased, but not pushed?

You are right! Fixed.

Some implication found:

The INDEX/R-INDEX tests now existing in the testcases assume the error message is:

The third argument to INDEX or R-INDEX must be in the range 1 to 32000 (for CHARACTER), 1 to 1073741823 (for LONGCHAR) or between 1 and the length of the MEMPTR (for INDEX). (4687)

while in OE 11.6 and 11.7 I get:

The third argument to INDEX or R-INDEX must be in the range 1 to 32000 (for CHARACTER) or 1 to 1073741823 (for LONGCHAR). (4687)

I suppose, the testcases were written for some more recent versions of OE, and we shall use the "old" message wich does not mention MEMPTR?

We MUST support both. Please add runtime version checking using the EnvironmentOps "Progress" version helpers.

Greg Shah wrote:

We MUST support both. Please add runtime version checking using the EnvironmentOps "Progress" version helpers.

The 11.6 and 11.7 both shows the "old" version.

The problem is I do not know which OE version is the current testcases compatible with.

Marian's team currently writes everything for 12.2 since that is what they have. That doesn't meant that that is the case for all test sets.

Marian: Please have your team document this in the details Redmine wiki page of each test set.

Greg Shah wrote:

Marian's team currently writes everything for 12.2 since that is what they have. That doesn't meant that that is the case for all test sets.

To be more specific it's 12.2.0, we also had issues with OE messages being updated in between service packs - some tests were not passing anymore in 12.2.2 :(

I'm thinking about excluding the check on exact error message from the 'core' tests and maybe have a separate test package for that kind of validation. This can be easily done by changing the AssertExt class, still under consideration but something should probably done sooner than later.

Marian: Please have your team document this in the details Redmine wiki page of each test set.

We can specify the OE version used on each test set... only if there are, and sure will be, additions to the same test set that are probably written in another OE version how this will work out?

I know it isn't nice to write, but in the 4GL code we can use PROVERSION and have conditional matching code. On the FWD side, when we find error messages that are different by version, then we should do the same thing using EnvironmentOps.

The bottom line here: we have customers migrating from multiple OE versions and we must be able to behave as a specific OE version based on runtime configuration. We cannot just code everything to 11.x or 12.x. This means the FWD and the 4GL tests need to be aware of these differences where they exist.

Greg Shah wrote:

I know it isn't nice to write, but in the 4GL code we can use PROVERSION and have conditional matching code. On the FWD side, when we find error messages that are different by version, then we should do the same thing using EnvironmentOps.

The bottom line here: we have customers migrating from multiple OE versions and we must be able to behave as a specific OE version based on runtime configuration. We cannot just code everything to 11.x or 12.x. This means the FWD and the 4GL tests need to be aware of these differences where they exist.

I've modified the TestIndex/TestRIndex tests, so they use different expected error message depending on the PROVERSION value. Now both tests succeed in testcases.

Greg Shah wrote:

I know it isn't nice to write, but in the 4GL code we can use PROVERSION and have conditional matching code. On the FWD side, when we find error messages that are different by version, then we should do the same thing using EnvironmentOps.

The bottom line here: we have customers migrating from multiple OE versions and we must be able to behave as a specific OE version based on runtime configuration. We cannot just code everything to 11.x or 12.x. This means the FWD and the 4GL tests need to be aware of these differences where they exist.

Does this mean we shall make FWD runtime honor the version returned by EnvironmentOps?

Does this mean we shall make FWD runtime honor the version returned by EnvironmentOps?

Yes, exactly this. Luckily, this is a pretty rare case because they try to keep OE backwards compatible. But we must handle it when it happens.

Greg Shah wrote:

Does this mean we shall make FWD runtime honor the version returned by EnvironmentOps?

Yes, exactly this. Luckily, this is a pretty rare case because they try to keep OE backwards compatible. But we must handle it when it happens.

Implemented in rev. 15581.

Greg Shah wrote:

Yes, exactly this. Luckily, this is a pretty rare case because they try to keep OE backwards compatible. But we must handle it when it happens.

Actually there is no such a thing as backwards compatible for error messages :)

In fact those can be changed by user and they used to provide localised versions in different languages - https://community.progress.com/s/article/19758

Imho FWD should use a similar approach, properties files are just fine, and 'hide' this error format mechanism inside ErrorManager and stop using the actual error messages in FWD codebase. Cluttering that even further based on 'proversion' is not going to help but it might be just me.

Now, related to the test cases - we've seen those messages changing (albeit slightly) even between service packs - 12.2.0 vs 12.2.14. What we are going to do to address this is use the 'promsgs' of version 12.2.0, same thing we've did for OO libraries provided by Progress. For us is impossible to write those tests so the error messages matches on every Progress version, we do not have older versions available to begin with.

As said, I would rather extract the assertions on the exact error message in separate test cases and only make sure the same errors are raised - as in size, order and error code/number - and don't bother with the exact error message when testing functionality.

Marian Edu wrote:

Greg Shah wrote:

Yes, exactly this. Luckily, this is a pretty rare case because they try to keep OE backwards compatible. But we must handle it when it happens.

Actually there is no such a thing as backwards compatible for error messages :)

Most messages don't ever change. It is only a small number that may change.

In fact those can be changed by user and they used to provide localised versions in different languages - https://community.progress.com/s/article/19758

Yuck. I'm not OK with error messages being dependent upon files independently installed or edited in the filesystem.

Imho FWD should use a similar approach, properties files are just fine,

I'm not as convinced. Whatever we do here, the content will be in the FWD jar files not in the filesystem.

and 'hide' this error format mechanism inside ErrorManager and stop using the actual error messages in FWD codebase.

We are already moving to this. I'm open to alternatives for how we encode and internationalize these, but the content must be in our jars.

Cluttering that even further based on 'proversion' is not going to help but it might be just me.

I get that but in practice, I don't think this is an issue for most messages.

Now, related to the test cases - we've seen those messages changing (albeit slightly) even between service packs - 12.2.0 vs 12.2.14. What we are going to do to address this is use the 'promsgs' of version 12.2.0, same thing we've did for OO libraries provided by Progress. For us is impossible to write those tests so the error messages matches on every Progress version, we do not have older versions available to begin with.

If we have changes from messages we already support, then we will need to version-ize them so that we don't break existing applications.

As said, I would rather extract the assertions on the exact error message in separate test cases and only make sure the same errors are raised - as in size, order and error code/number - and don't bother with the exact error message when testing functionality.

I'm fine with this. Please note that we have application level testing (e.g. ChUI regression tests) that depend on specific error messages.

Greg Shah wrote:

Yuck. I'm not OK with error messages being dependent upon files independently installed or edited in the filesystem.

It isn't like that, although everything is in the filesystem... the error messages are available in plain text in 'prohelp/msgdata' (split in chunk of 50 messages/file), at runtime the file used is 'promsgs' which is a binary file ('encoded' with 'Strings') - [[https://community.progress.com/s/article/How-to-view-promsgs-contents]]

I'm not as convinced. Whatever we do here, the content will be in the FWD jar files not in the filesystem.

It can be anywhere if you don't trust the users, only in 4GL there is also a statement that can be used to change the messages file - `PROMSGS` [[https://documentation.progress.com/output/ua/OpenEdge_latest/pdsoe/PLUGINS_ROOT/com.openedge.pdt.langref.help/rfi1424920722617.html]]

As said, I would rather extract the assertions on the exact error message in separate test cases and only make sure the same errors are raised - as in size, order and error code/number - and don't bother with the exact error message when testing functionality.

I'm fine with this. Please note that we have application level testing (e.g. ChUI regression tests) that depend on specific error messages.

Agreed, only for functional tests I would say validating the error numbers should be enough... in any case if anyone is building some error handling layer it should only rely on the error numbers not the actual error messages (see the PROMSGS statement).

Can the 8309a branch be merged into trunk?
The change is already over a year old. Merging it directly into the current trunk would require significant effort due to the broken state of the trunk repository.
Instead, I will create a new branch and perform a semi-manual merge of the differences.