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

Namsonx/task/stabi branch #238

Merged
merged 48 commits into from
Apr 2, 2024
Merged

Conversation

namsonx
Copy link
Collaborator

@namsonx namsonx commented Mar 13, 2024

Hello Thomas, Hello Holger,

I create this pull-request to remove globals scope out of all exec() executions as you mentioned before to avoid some risks may be appear in future.
I defined self.JPGlobals = {} in the __init__ of CJsonPreprocessor to manage root parameters in json files.

Thank you,
Son

Copy link
Owner

@test-fullautomation test-fullautomation left a comment

Choose a reason for hiding this comment

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

Hi @namsonx ,
thank you for your pull-request.
Can you please also update release_info file from enduser perspective.
Like this and that is working now with short example...

Thank you,
Thomas

Copy link
Collaborator

@HolQue HolQue left a comment

Choose a reason for hiding this comment

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

Hi Son,

I do not understand this. In
test-fullautomation/robotframework-testsuitesmanagement#251
I asked a question. The question is not answered. The behavior of the JsonPreprocessor w.r.t. 251 is the same like before.

What is the status now?

@namsonx
Copy link
Collaborator Author

namsonx commented Mar 13, 2024

Hi Son,

I do not understand this. In test-fullautomation/robotframework-testsuitesmanagement#251 I asked a question. The question is not answered. The behavior of the JsonPreprocessor w.r.t. 251 is the same like before.

What is the status now?

Hello Holger,

Sorry for the confusion, this is my mistake, this pull-request is not related to your question in TSM ticket 251.
Please ignore my comment about about TSM ticket 251.

Thank you,
Son

@test-fullautomation test-fullautomation added enhancement New feature or request 0.11.0 labels Mar 18, 2024
@test-fullautomation test-fullautomation added this to the 0.11.0 milestone Mar 18, 2024
@test-fullautomation
Copy link
Owner

Hi @namsonx ,
if you provide a release_info file (from enduser perspective. e.g. this and this is possible now - with small code example - I would merge.
Than you,
Thomas

@HolQue
Copy link
Collaborator

HolQue commented Mar 22, 2024

Hi Son,

the wording needs to be adapted:

Not: Reason: Empty or special character is detected in pair of square brackets.
But: Reason: A pair of square brackets is empty or contains not allowed characters.

Not: Slicing is currently not supported!
But: Slicing is not supported!

(no need to mention 'currently', because then customers will assume that slicing might be available in near future already)

Not: 'Invalid expression while handling the parameter '${listP.${keyP}}'.'
But: 'Invalid expression found: '${listP.${keyP}}'.'

But nevertheless: This expression itself is not really invalid. It's a data type issue: List indices are expected to be positive integers, and not strings. Later this should be reworked.

02.04.2024
Retest successful.

@namsonx
Copy link
Collaborator Author

namsonx commented Mar 22, 2024

Hi Son,

the wording needs to be adapted:

Not: Reason: Empty or special character is detected in pair of square brackets. But: Reason: A pair of square brackets is empty or contains not allowed characters.

Not: Slicing is currently not supported! But: Slicing is not supported!

(no need to mention 'currently', because then customers will assume that slicing might be available in near future already)

Not: 'Invalid expression while handling the parameter '${listP.${keyP}}'.' But: 'Invalid expression found: '${listP.${keyP}}'.'

But nevertheless: This expression itself is not really invalid. It's a data type issue: List indices are expected to be positive integers, and not strings. Later this should be reworked.

Hello Holger,

Thank you for your review, I updated error messages as your suggestion

@HolQue
Copy link
Collaborator

HolQue commented Mar 26, 2024

Hi Son,

a tiny wish only:

Please remove the part "Please update the expression" from following error message:

'Slicing is not supported! Please update the expression '${testlist}[1:]'.'

In case of an error, it is obvious that the user has to do something to avoid this error. There is no need to mention this explicitly. Otherwise you would have to add such a statement to every error message.

Let's keep it short:

'Slicing is not supported (expression: '${testlist}[1:]').'

Copy link
Collaborator

@HolQue HolQue left a comment

Choose a reason for hiding this comment

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

Hi Son,

this error message is rather long winded (three times 'must'; this is too much):

Invalid key name detected: '"par\\am"'. Key names in JSON objects must adhere to the following rules: they must be strings enclosed in double quotes, must not contain control characters.

It is s good idea to give a recommendation how to solve the error, but it is also very difficult in my opinion, to compress the entire JSON naming convention to one single and short sentence.

I made a cross check with the original JSON interface. This interface throws errors like that:

Invalid \escape: line ...
Invalid control character at: line ...

This gives enough information to enable users to find the error.

Therefore I suggest to pass the identified expression at first directly to the Python JSON interface (json.loads(...)) and let this interface do the naming convention check. JsonPreprocessor specific checks may follow.

Finally an error message can be like this:

Invalid control character at: line ... Key name is invalid: '"par\\am"'

(like I already mentioned in an email: at first the error itself, at second the consequence)

The naming convention check is mentioned in the history of the TestsuitesManagement, but not in the history of the JsonPreprocessor - and should be added there.

Copy link
Owner

@test-fullautomation test-fullautomation left a comment

Choose a reason for hiding this comment

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

Hi Son,
looks good to me!
Thank you,
Thomas

@test-fullautomation test-fullautomation merged commit 60e326c into develop Apr 2, 2024
3 checks passed
This was referenced Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.11.0 enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

3 participants