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

Convert jstests to selenium: deletecell.js #3465

Merged

Conversation

@sheshtawy
Copy link
Contributor

@sheshtawy sheshtawy commented Mar 24, 2018

This PR is related to #3335. It's mean to convert the file deletecell.js into a selenium test.

@sheshtawy sheshtawy changed the title [WIP] Convert jstests to selenium: deletecell.js Convert jstests to selenium: deletecell.js Apr 2, 2018
notebook.current_cell.send_keys('dd')

def test_delete_cells(notebook):
print('testing deleteable cells')
Copy link
Member

@takluyver takluyver Apr 3, 2018

Choose a reason for hiding this comment

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

We shouldn't need this print() - pytest should tell us which test is which when it runs them.

Loading

@takluyver
Copy link
Member

@takluyver takluyver commented Apr 3, 2018

Thanks, it looks like the tests are passing now. Are you satisfied that you've converted the whole of deletecell.js? If so, you can delete that file as part of this PR - the aim is to get rid of the Phantom tests as well as creating Selenium ones.

Loading


@pytest.fixture
def notebook(authenticated_browser):
return Notebook.new_notebook(authenticated_browser)

def get_cells_contents(nb):
Copy link
Contributor Author

@sheshtawy sheshtawy Apr 3, 2018

Choose a reason for hiding this comment

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

@takluyver Thanks for taking the time to review my PR. I made some changes to address your comments. I have only one more idea that I'm not sure about and I would like your input. I was thinking of moving this function to the Notebook class since I think it might be needed/repeated in several tests. I'm not sure about other helper functions like delete_cell, cell_is_deletable and set_cell_metadata, do you think they also should be moved to the Notebook class too?

Loading

Copy link
Member

@takluyver takluyver Apr 4, 2018

Choose a reason for hiding this comment

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

I'd say that the get_cells_contents and set_cell_metadata can move to the notebook class, but let's leave delete_cell and cell_is_deletable here for now. They can always be moved in another PR if we change our minds. :-)

Loading

Copy link
Contributor Author

@sheshtawy sheshtawy Apr 4, 2018

Choose a reason for hiding this comment

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

I made the changes. I believe the PR is ready to be merged if you don't have any more comments :)

Loading

@takluyver takluyver added this to the 5.5 milestone Apr 4, 2018
@takluyver
Copy link
Member

@takluyver takluyver commented Apr 4, 2018

Thanks!

Loading

@takluyver takluyver merged commit e3ee807 into jupyter:master Apr 4, 2018
4 checks passed
Loading
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants