Feature/war 991: Implement wtag feature at framework level#74
Conversation
…to master * commit '2ea70552e0043d0691f50ce901f1289b80047341': Updated default_config Updated default_config Excluded warriorframework tag from being processed Changed the default config file
Annam16
left a comment
Annam16
left a comment
There was a problem hiding this comment.
Travis CI build failed. Please resolve it
…framework into feature/WAR-991
navvrnd
changed the title
|
@navvrnd Please resolve merge conflicts |
| def get_values_from_datafile(self, system_name, strvar, langs, states, | ||
| currencys, ramspace, configfile, intvar, | ||
| file_config): | ||
| """get the values from datafile |
There was a problem hiding this comment.
@navvrnd Add description for each argument in documentation string
| <Time>15:51</Time> | ||
| <State/> | ||
| <InputDataFile>../../data/data_wtag.xml</InputDataFile> | ||
| <!-- <InputDataFile>../../../warrior_fnc_tests/warrior_tests/data/framework_tests/data_wtag.xml</InputDataFile> --> |
There was a problem hiding this comment.
please remove this commented line since it contains our local repository name.
| """ | ||
| vartype = type(var) | ||
| if vartype is not datatype: | ||
| print_error('is not a {}, but {}'.format(datatype, vartype)) |
There was a problem hiding this comment.
Can you please add the error message more clear?
satyakrish
left a comment
satyakrish
left a comment
There was a problem hiding this comment.
the resolution provided to intial comments was not sufficient, updated more details on the questions.. please address them.
| else: | ||
| return value | ||
| else: | ||
| return None |
There was a problem hiding this comment.
@navvrnd
for #1 i know it is the place where you are converting wtag var to actual value ... i had asked the logic behind the if/else statements you had used .. why have you used in a specific flow? Nevertheless I can see that in your if/else logic if system name is not the first argument of the keyword wtag substitution will fail.. please fix it.
for #2: I still dont understand your explanation.. please explain here with a small example on what line 196 is doing both if and else part
for #3: your suggestion is not an advisable practice but it works, i'm ok with it but I would advice to move it to the constructor with default values (you can default it to None and in the function check if it none and if so call get credentials to get tag_dict in your function and re assign it to None when you are done with it)
#4 ok with your implementation
…framework into feature/WAR-991
satyakrish
left a comment
satyakrish
left a comment
There was a problem hiding this comment.
Updates from call on 12/20:
#1: Arvind to handle the case when system name is not the first argument. wtag should work even if system name is not the first argument.
#2: Lets stick with existing behaviors , please remove the additional checks introduced as part of wtag.
existing behavior:
-
if an argument is mandatory and no value provided in testcase step then the keyword should fail.
-
if an argument is optional and no value provided in test case step then the default value in keyword signature should be used.
…framework into feature/WAR-991
…argument to be from wtags if not provided
|
@satyakrish made the changes as per your comments. Please review and approve. Logs attached in the ticket. |
|
The changes look good, but while reviewing it this time I observed another issue. With the currently implementation do we assume that a keyword would always have mandatory arguments? If so we should make changes to implement the case where the keyword has all optional arguments as well. Because there is no check to stop a developing a keyword which has all optional arguments including the system name. Can you verify this case and make changes accordingly. |
|
@satyakrish cc: @bjrr Made the requested changes. Please check and approve. |
|
@navvrnd : the changes you made are not sufficient? The purpose of the change was to make sure that if a keyword has all optional arguments , wtag substitution should work fine? but you have not covered it at all... You have updated your keyword to have all optional arguments. 2- you should also test the case where
|
|
@satyakrish Made the required changes and updated the ticket with the cli full regression, wtag and kw prefix logs. Please check and approve. |
6 participants
Please check the design doc from the jira ticket. Please note that file option would not work here as expected, as it is implemented separately in war-1172.