Skip to content
This repository has been archived by the owner on Oct 21, 2020. It is now read-only.

Started implementation of wrapping some Selenium JS commands #70

Merged
merged 5 commits into from
Mar 1, 2019

Conversation

nadvolod
Copy link
Collaborator

No description provided.

@codecov-io
Copy link

codecov-io commented Feb 22, 2019

Codecov Report

Merging #70 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff          @@
##           master   #70   +/-   ##
====================================
  Coverage       0%    0%           
====================================
  Files           3     3           
  Lines         201   201           
  Branches       18    18           
====================================
  Misses        201   201

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4ff3ad...105c121. Read the comment docs.

Copy link
Contributor

@mmerrell-sauce mmerrell-sauce left a comment

Choose a reason for hiding this comment

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

Go ahead and change that access level and I think this will be ready to merge. I'm trying to resist the urge to build a map of "sauce:XYZ" strings that would help abstract the stuff that might change, but honestly I don't think it would make things better in this case.

import org.junit.Test;
import static org.junit.Assert.*;


public class SauceHelperTests {
SauceHelper sauceHelper;
Copy link
Contributor

Choose a reason for hiding this comment

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

The Codacy check will pass if you make this private instead of package-protected (that's the access level when you leave it out)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, it might make sense to create that map. But I don't know how much extra benefit it will add over having each of the strings in a separate method. I guess if we move the map into it's own class, that will prevent SauceHelper.java from changing if we do need to update those strings. But also, feels a little like premature optimization to me as I've seen no proof that this data will change. Hence, I kept it simple until we have more information on the behavior of our system.

I will make that Codacy fix

@nadvolod nadvolod merged commit ae187ab into master Mar 1, 2019
@nadvolod nadvolod deleted the js-executor branch March 1, 2019 16:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants