WAR-1348 : Modularise runmode across Warrior. As a user, I want same tags for runmode wherever I use runmode functions such as RMT, RUP & RUF by brettleehari · Pull Request #98 · warriorframework/warriorframework

brettleehari · 2017-07-28T15:37:15Z

When user uses RMT in suite level, the option is ATTEMPTS.
But when users do not use the tag MAX_ATTEMPTS which is used for RUP&RUF SUITES fail to run.
REmoved the logic based on MAX_ATTEMPTS and simplified the code.

Added a test with SUITE level RMT.

…id not give maxattempts

satyakrish · 2017-07-28T18:56:23Z

-        else:
-            execution_value = Max_Attempts
-
+        execution_value = Utils.xml_Utils.getChildAttributebyParentTag(testsuite_filepath, 'Details', 'type', 'Number_Attempts')


please add a check here to see if execution value is a valid integer otherwise throw a warning and default it to 1 so that the test runs only once

Hi @satyakrish,
I agree to the above improvement comment that we have to evaluate whether user has provided numeric value to handle user error. It is an improvement from our current behavior where we trust that user provide whole numbers.
I feel above suggestion is not a clean solution, currently there is bug and this ticket just fixes that bug to unblock the user.
While working on the issue found few anomalies Kindly consider below points and if you still feel you want me to check value for RMT is integer i shall do it:

Just as you suggested change for RMT in SUITE global level, RUP & RUF requires changes too.

We support Runmode in SUITE step level and we would require similar changes here too.

We support Runmode in TESTCASE GLOBAL and STEP level and requires same changes.

Although we support RMT, RUP, RUF at 4 different levels(as mentioned above) their functionalities are the same but we use maxattempts, Number_attempts xml tags in SUITE, value tag in Testcases.

Given the above 4 points, I propose two changes and be handled as an improvement item:

RUNMODE functionalities shall carry the same tag across Warrior since the functionalities are exactly same. Slowly we can deprecate older tags or have both.

Instead of handling user error such as validating whether a value is a whole number. How about using Django form fields to restrict users to only fill numeric else throw error. Because as a user I do not want to find out my user errors while executing but while developing.

@brettleehari

initial commit #1: we should go with both for backward comatibility, i dont think we can get rid of one at this moment.

Add documentations for Actions and Framework Utils #2: Agree that the error handling should be applied for all the cases like rup, ruf, rmt.
Where should we do it? data validation should be done at the front end for sure.. but we should not totally ignore error handling at warrior level. Because even today there are lot of users who use warrior directly without using the UI.
I would suggest to raise a ticket for error handling at UI, and handle errors in warrior using this ticket.
If we encounter an un supported value then we should raise a warning that prints out what was expected and what was received and that default value will be used instead with the default value printed.

satyakrish

refer to comments

…ior to be uniform

brettleehari · 2017-08-10T14:42:01Z

Hello reviewers,
Work in progress, Please do not review for now. I shall let you know.

8/30 Ka
@brettleehari Looks like there are new commits pushed, is this ready to review now?

…nmode in Suite global level

kaheichan · 2017-08-31T02:58:57Z

+    root = Utils.xml_Utils.getRoot(filepath)
+    steps = root.find(step_tag)
+    if steps is None:
+        print_warning("The file: '{}' has no steps "


can replace the word steps with value of step_tag

Oops...What I mean is...step_tag is an xml element
and in the element there is a value specifying whether it is a step/testcase/testsuite
so the warning message can correctly say
The file X has no step/testcase/testsuite to be executed

@kaheichan Done!

kaheichan · 2017-08-31T03:18:39Z

+                else:
+                    for i in range(0, value):
+                        copy_step = copy.deepcopy(step)
+                        copy_step.find("runmode").set("attempt", i + 1)


I think I wrote this more than 1 year ago...but even if this is the 1st step, it will need to set the value to next also

I didnt get your point.

For step_list length > 1
there is a line
copy_step.find("runmode").set("value", go_next)
but it doesn't exist when step_list is empty

kaheichan · 2017-08-31T03:20:37Z

+                    for i in range(0, retry_value):
+                        copy_step = copy.deepcopy(step)
+                        copy_step.find("retry").set("attempt", i + 1)
+                        step_list_with_rmt_retry.append(copy_step)


line 48-81 contain 4 blocks of similar code
Since we are reworking the functionality
we should also wrap them in a common function/code block and reuse

kaheichan · 2017-08-31T03:30:09Z

+                                str(test_suite_status).upper() == "ERROR":
+                    break
+
+    #The below ELIF is to preserve backward compatibility. The new logic is available/


If the value of execution_type hasn't changed, why would the runmode logic not handled by the original code?

The Runmode is under execution type in the current design. So, I am not changing anything to the current logic to preserve backward compatibility. To bring uniformity across runmodes, I have introduced a tag similar to the tags available for runmode in suite_step, Project_step & Testcase_step in the Suite Global tags. Going forward users would see the new tags through Katana (This would be similar to all runmode options across Warrior).
If this explanation doesn't help, I shall call you tomorrow to discuss.

kaheichan

The code looks good
Just need to address several format/logical comment

9/16-Hari
@kaheichan Done with all your recommendation.

…new def

…t is added

kaheichan · 2017-09-22T19:16:26Z

+    root = Utils.xml_Utils.getRoot(filepath)
+    steps = root.find(step_tag)
+    if steps is None:
+        print_warning("The file: '{}' has no steps "


Oops...What I mean is...step_tag is an xml element
and in the element there is a value specifying whether it is a step/testcase/testsuite
so the warning message can correctly say
The file X has no step/testcase/testsuite to be executed

@kaheichan Done!

kaheichan · 2017-09-22T19:30:48Z

+            runmode, value = get_runmode_from_xmlfile(step)
+            retry_type, _, _, retry_value, _ = get_retry_from_xmlfile(step)
+            if runmode is not None and value > 0:
+                if len(step_list) > 1:


Looks like L70-79 are duplicated, any reason to keep both?

kaheichan · 2017-09-22T19:31:17Z

+                                                                value, go_next, mode="runmode",
+                                                                tag="value")
+            if retry_type is not None and value > 0:
+                if len(step_list) > 1:


…, suite or a project

…/warriorframework into bugfix/WAR-1348

kaheichan · 2017-10-04T19:10:42Z

@brettleehari Looks like there is a merge conflict

@kaheichan Resolved the Merge conflicts, think it will be more prevalent as we are merging many tickets. Kindly review.

transferred review to ka

…relatively addressed the new items

kaheichan · 2017-10-09T19:28:42Z

Ran the test and verified it work

bjrr · 2017-10-13T23:30:39Z

@kaheichan Is there a Katana change needed for this ticket?

kaheichan

Upon going through the design again, has some questions come up, need sometimes to formalize the questions, please don't merge this now

brettleehari · 2017-10-24T09:29:21Z

@kaheichan Can you please share your reasons for rejecting the PR you approved. It would help me to resolve it at the earliest.

brettleehari · 2017-10-25T09:31:39Z

Dear @kaheichan,
Gentle reminder-2. This ticket is part of 3.5.0. Humbly Request to share comments so that I can work on this and deliver this item for the team.

kaheichan · 2017-10-26T00:07:38Z

Hi @brettleehari
A detailed comment has been made in the Jira ticket, please go ahead and check it out there

To summarize, please deliver bug fix and code improvements in 2 separate PR
Also if you decide to use this PR as the bugfix PR, please update the description of this PR as it is outdated

satyakrish · 2017-11-30T21:48:57Z

transferred review to Ka on oct-6 hence removing myself.

sanikakulkarni · 2017-11-30T23:23:20Z

Pull Request has been approved by @venkat1077 which was later dismissed on account of new commits.

kaheichan · 2018-04-13T12:25:02Z

Github is preventing me from clearing the requested change...

ReshmaManohar · 2018-07-11T05:13:01Z

Closing this PR. Will open a new PR for this issue.

removed maxattempts logic in RMT, which was causing issue when user d…

3d967be

…id not give maxattempts

brettleehari requested review from Annam16, bjrr and satyakrish July 28, 2017 15:37

satyakrish reviewed Jul 28, 2017

View reviewed changes

satyakrish previously requested changes Jul 28, 2017

View reviewed changes

sudharshan, hariprasad added 4 commits July 30, 2017 15:57

removing the print instead of print utils

d32bdae

Modularise getting list of suite/step/cases

0211703

Created runmode at suite global level to make all runmode across warr…

2ec871d

…ior to be uniform

resolve merge conflicts

4b28fd9

brettleehari added the enhancement label Aug 11, 2017

brettleehari requested review from kaheichan and venkat1077 August 18, 2017 16:15

sudharshan, hariprasad added 2 commits August 21, 2017 05:53

improve pylint to pass travis

e293eae

Add tests for Run mode in Step level at Project, Suite, Testcase & Ru…

19963ec

…nmode in Suite global level

venkat1077 previously approved these changes Aug 23, 2017

View reviewed changes

kaheichan reviewed Aug 31, 2017

View reviewed changes

kaheichan suggested changes Aug 31, 2017

View reviewed changes

sudharshan, hariprasad added 3 commits September 4, 2017 01:43

address review comments, modularized the frequently used code with a …

a67e6f2

…new def

further modularized the repeatable code, new function append step lis…

f687b87

…t is added

improve pylint

b5a827e

brettleehari dismissed venkat1077’s stale review via a67e6f2 September 6, 2017 15:04

kaheichan suggested changes Sep 22, 2017

View reviewed changes

sudharshan, hariprasad and others added 3 commits September 28, 2017 07:29

removed the repeating code

1691d4f

Providing user with more appropriate warning whether it is a testcase…

feeab22

…, suite or a project

Merge branch 'develop' into bugfix/WAR-1348

197bf91

sudharshan, hariprasad added 2 commits September 30, 2017 11:08

corrected the xml element in line 63

1dffc1b

Merge branch 'bugfix/WAR-1348' of https://github.com/warriorframework…

81dd3bb

…/warriorframework into bugfix/WAR-1348

Merge branch 'develop' into bugfix/WAR-1348

1c26886

due to new merges to develop the pylint score of my code has reduced …

8a53409

…relatively addressed the new items

kaheichan approved these changes Oct 9, 2017

View reviewed changes

kaheichan suggested changes Oct 18, 2017

View reviewed changes

bjrr changed the title ~~Bugfix/war 1348: RMT expects the tag MAX_ATTEMPTS and fails.~~ WAR-1348: RMT expects the tag MAX_ATTEMPTS and fails. Oct 24, 2017

sanikakulkarni assigned brettleehari Nov 27, 2017

sanikakulkarni removed the request for review from Annam16 November 30, 2017 00:36

satyakrish requested review from satyakrish and removed request for satyakrish November 30, 2017 15:55

brettleehari and others added 2 commits December 21, 2017 00:02

Merge branch 'develop' into bugfix/WAR-1348

d9673ae

Merge branch 'develop' into bugfix/WAR-1348

c913d5c

kaheichan requested review from kaheichan and removed request for kaheichan April 13, 2018 12:22

adding minor changes

1c99814

ReshmaManohar changed the title ~~WAR-1348: RMT expects the tag MAX_ATTEMPTS and fails.~~ WAR-1348 : Modularise runmode across Warrior. As a user, I want same tags for runmode wherever I use runmode functions such as RMT, RUP & RUF May 3, 2018

adding * support to get_step_list

ad62ab7

ReshmaManohar closed this Jul 11, 2018

Uh oh!

Conversation

brettleehari commented Jul 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettleehari Jul 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

satyakrish Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

satyakrish left a comment

Choose a reason for hiding this comment

Uh oh!

brettleehari commented Aug 10, 2017 • edited by kaheichan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaheichan Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaheichan Sep 22, 2017 • edited by brettleehari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaheichan left a comment • edited by brettleehari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaheichan Sep 22, 2017 • edited by brettleehari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaheichan commented Oct 4, 2017 • edited by brettleehari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaheichan commented Oct 9, 2017

Uh oh!

bjrr commented Oct 13, 2017

Uh oh!

kaheichan left a comment

Choose a reason for hiding this comment

Uh oh!

brettleehari commented Oct 24, 2017

Uh oh!

brettleehari commented Oct 25, 2017

Uh oh!

kaheichan commented Oct 26, 2017

Uh oh!

satyakrish commented Nov 30, 2017

Uh oh!

sanikakulkarni commented Nov 30, 2017

Uh oh!

kaheichan commented Apr 13, 2018

Uh oh!

brettleehari Jul 30, 2017 •

edited

Loading

satyakrish Jul 31, 2017 •

edited

Loading

brettleehari commented Aug 10, 2017 •

edited by kaheichan

Loading

kaheichan Aug 31, 2017 •

edited

Loading

kaheichan Sep 22, 2017 •

edited by brettleehari

Loading

kaheichan left a comment •

edited by brettleehari

Loading

kaheichan Sep 22, 2017 •

edited by brettleehari

Loading

kaheichan commented Oct 4, 2017 •

edited by brettleehari

Loading