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

Feature/robot bge automation #3933

Merged
merged 135 commits into from
Mar 11, 2019
Merged

Feature/robot bge automation #3933

merged 135 commits into from
Mar 11, 2019

Conversation

skristem
Copy link
Contributor

@skristem skristem commented Dec 20, 2018

  • Added automation code for BGE

Critical Changes

Changes

Issues Closed

New Metadata

Deleted Metadata

Copy link
Contributor

@boakley boakley left a comment

Choose a reason for hiding this comment

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

One general comment I want to make is that about half of the keywords don't have any documentation. While some are self-explanatory and probably obvious to you right now, it will be difficult for others to use these keywords without having to read the code.

I would like to recommend that we require documentation for every keyword that is in a library or resource file. I realize that some keywords don't require much documentation (eg: click record button is fairly self-explanatory), but even in those cases a simple one sentence explanation is better than nothing at all.

It's important to remember that keywords in a library or resource file are intended to be reused. We need to make reuse as easy as possible, and robust documentation gets us closer to that goal.

@@ -704,7 +687,7 @@ def select_value_from_list(self,list_name,value):
self.selenium.select_from_list_by_label(loc,value)

def select_value_from_bge_dd(self,list_name,value):
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "dd" stand for in this context? Dropdown? Date Dialog? Something else?

Execute JavaScript window.document.evaluate('${xpath}', document, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue.scrollIntoView(true)
Select Duellist Values Payment Selected Fields Check/Reference Number
Click Duellist Button Payment Move selection to Available Fields
Click Element With Locator bge.duellist Opportunity Available Fields Donation Name
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this block of changes. While it might have made the test slightly easier to read with some highly specialized keywords (select duellist values, click duellist button, etc), I think having one general purpose keyword instead of multiple special-purpose keywords will help make it easier to learn and use the NPSP keywords.

Copy link
Contributor

@boakley boakley left a comment

Choose a reason for hiding this comment

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

There still seems to be some keywords without documentation, but I don't want to hold up a merge just because of that so I'm going to approve my review.

@skristem skristem merged commit ccecc5d into master Mar 11, 2019
@skristem skristem deleted the feature/robot__bge-automation branch March 11, 2019 21:39
@sam-knox
Copy link
Contributor

@melissabarber - Can you take a look at the release notes for this PR?

@davisagli
Copy link
Contributor

@sam-knox @melissabarber no release notes for this one, it's updates to test automation

@davisagli
Copy link
Contributor

@melissabarber oh. which is the edit you made. carry on :)

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.

6 participants