Skip to content

Conversation

@xuscrbw
Copy link
Contributor

@xuscrbw xuscrbw commented Nov 16, 2020

Added a RStudio object and RStudioPage class to help run the test.
Right now, this WIP only has a generic: open the proxy, set a value to a variable, verify the variable shows under the Global Environment section.

Will probably do some more work to add in a more specific CSS Selector and see if there's anything else that we can do.

Have you read CONTRIBUTING.md lately? If not, do that first.

I, the developer opening this PR, do solemnly pinky swear that:

  • I've documented my API changes in Swagger

In all cases:

  • Get a thumbsworth of review and PO signoff if necessary
  • Verify all tests go green
  • Squash and merge; Delete your branch after this
  • Test this change deployed correctly and works on dev environment after deployment

@xuscrbw
Copy link
Contributor Author

xuscrbw commented Nov 16, 2020

jenkins retest


class RStudioMainPage(override val url: String)(implicit override val authToken: AuthToken,
implicit override val webDriver: WebDriver)
extends JupyterPage
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wondering if we should rename JupyterPage to ToolPage or something if it's meant to be abstract

import org.openqa.selenium.WebDriver
import org.broadinstitute.dsde.workbench.leonardo.notebooks.{JupyterPage, NotebooksListPage}

class RStudioPage(override val url: String)(implicit override val authToken: AuthToken,
Copy link
Collaborator

@rtitle rtitle Nov 17, 2020

Choose a reason for hiding this comment

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

Curious what the difference is between RStudioMainPage and RStudioPage -- I thought there was only 1 RStudio page (contrast with Jupyter which has the "list" page and the "notebook" page).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially copied over the Notebook format because I was running into some issues running the test, but I think it was separate. Removing the extra RStudioMainPage

// // See this ticket for adding more comprehensive selenium tests for RStudio:
// // https://broadworkbench.atlassian.net/browse/IA-697
Thread.sleep(10000)
rstudioPage.pressKeys("varA <- 1000")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome!! does this work? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, looks like it does work. I do want to see if there's a more precise selector for the global environment table, but Keys does seem to work as intended

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More specifically, press keys works here because the console is already active when we get onto the page. There may be some more complexity in typing into other areas of the screen, but for now this should satisfy what we need

@xuscrbw xuscrbw force-pushed the wx-ia-697 branch 2 times, most recently from d670eeb to d976231 Compare November 18, 2020 14:40
@xuscrbw
Copy link
Contributor Author

xuscrbw commented Nov 18, 2020

jenkins retest

@xuscrbw xuscrbw changed the title WIP: IA-697: adding Rstudio smoke test IA-697: adding Rstudio smoke test Nov 18, 2020
@xuscrbw xuscrbw force-pushed the wx-ia-697 branch 3 times, most recently from 8ed4823 to ef2bbdb Compare November 19, 2020 13:59
Thread.sleep(10000)
rstudioPage.pressKeys("varA <- 1000")
rstudioPage.pressKeys(Keys.ENTER.toString)
Thread.sleep(10000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious if the sleeps are needed.. wonder if there is a way to use Selenium to await conditions instead of hard-coding sleeps.

Copy link
Collaborator

@rtitle rtitle left a comment

Choose a reason for hiding this comment

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

This looks great! Approving this PR, we can iterate and add more RStudio tests as needed.

@xuscrbw
Copy link
Contributor Author

xuscrbw commented Nov 20, 2020

jenkins retest

@codecov
Copy link

codecov bot commented Nov 20, 2020

Codecov Report

Merging #1703 (b4b1537) into develop (63b3946) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1703   +/-   ##
========================================
  Coverage    76.71%   76.71%           
========================================
  Files          112      112           
  Lines         8038     8038           
  Branches       138      138           
========================================
  Hits          6166     6166           
  Misses        1872     1872           

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 63b3946...b4b1537. Read the comment docs.

@xuscrbw xuscrbw force-pushed the wx-ia-697 branch 2 times, most recently from 04219d1 to ab04174 Compare November 21, 2020 04:25
@xuscrbw
Copy link
Contributor Author

xuscrbw commented Nov 21, 2020

jenkins retest

@xuscrbw xuscrbw merged commit c80029c into develop Nov 23, 2020
@xuscrbw xuscrbw deleted the wx-ia-697 branch November 23, 2020 19:25
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.

3 participants