-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
takluyver
merged 17 commits into
jupyter:master
from
sheshtawy:convert-jstest-to-selenium-deletecell
Apr 4, 2018
Merged
Changes from 6 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
5677e22
Initial commit to convert deletecell.js to selenium
sheshtawy e24a573
Merge branch 'master' into convert-jstest-to-selenium-deletecell
sheshtawy 7b8db48
Add tests for deletable cells and non-deleteable cells
sheshtawy 6b1691d
Add tests to make sure copied cells are deletable
sheshtawy 5dbb2ac
Move notebook fixture to deletetest file to minimize the dependency o…
sheshtawy 2c5748f
Remove unnecessary comments
sheshtawy 30f7b8d
Merge remote-tracking branch 'origin/master' into HEAD
sheshtawy 413cca8
Delete tests
sheshtawy 14acb8d
Move some utility functions from the test module to the utils.py module
sheshtawy 835211f
Initial commit to convert deletecell.js to selenium
sheshtawy a0e89b6
Add tests for deletable cells and non-deleteable cells
sheshtawy 2ef45d5
Add tests to make sure copied cells are deletable
sheshtawy a47ecd4
Move notebook fixture to deletetest file to minimize the dependency o…
sheshtawy 5e57a51
Remove unnecessary comments
sheshtawy b097da2
Delete tests
sheshtawy bdfdd24
Move some utility functions from the test module to the utils.py module
sheshtawy ce2c3a9
Merge branch 'convert-jstest-to-selenium-deletecell' of github.com:Sh…
sheshtawy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
import os | ||
import pytest | ||
from .utils import Notebook | ||
|
||
@pytest.fixture | ||
def notebook(authenticated_browser): | ||
return Notebook.new_notebook(authenticated_browser) | ||
|
||
def get_cells_contents(nb): | ||
JS = 'return Jupyter.notebook.get_cells().map(function(c) {return c.get_text();})' | ||
return nb.browser.execute_script(JS) | ||
|
||
def set_cell_metadata(nb, index, key, value): | ||
JS = 'Jupyter.notebook.get_cell({}).metadata.{} = {}'.format(index, key, value) | ||
return nb.browser.execute_script(JS) | ||
|
||
def cell_is_deletable(nb, index): | ||
JS = 'return Jupyter.notebook.get_cell({}).is_deletable();'.format(index) | ||
return nb.browser.execute_script(JS) | ||
|
||
def delete_cell(notebook, index): | ||
notebook.focus_cell(index) | ||
notebook.to_command_mode | ||
notebook.current_cell.send_keys('dd') | ||
|
||
def test_delete_cells(notebook): | ||
print('testing deleteable cells') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
a = 'print("a")' | ||
b = 'print("b")' | ||
c = 'print("c")' | ||
|
||
notebook.edit_cell(index=0, content=a) | ||
notebook.append(b, c) | ||
notebook.to_command_mode() | ||
|
||
# Validate initial state | ||
assert get_cells_contents(notebook) == [a, b, c] | ||
for cell in range(0, 3): | ||
assert cell_is_deletable(notebook, cell) | ||
|
||
set_cell_metadata(notebook, 0, 'deletable', 'false') | ||
set_cell_metadata(notebook, 1, 'deletable', 0 | ||
) | ||
assert not cell_is_deletable(notebook, 0) | ||
assert cell_is_deletable(notebook, 1) | ||
assert cell_is_deletable(notebook, 2) | ||
|
||
# Try to delete cell a (should not be deleted) | ||
delete_cell(notebook, 0) | ||
assert get_cells_contents(notebook) == [a, b, c] | ||
|
||
# Try to delete cell b (should succeed) | ||
delete_cell(notebook, 1) | ||
assert get_cells_contents(notebook) == [a, c] | ||
|
||
# Try to delete cell c (should succeed) | ||
delete_cell(notebook, 1) | ||
assert get_cells_contents(notebook) == [a] | ||
|
||
# Change the deletable state of cell a | ||
set_cell_metadata(notebook, 0, 'deletable', 'true') | ||
|
||
# Try to delete cell a (should succeed) | ||
delete_cell(notebook, 0) | ||
assert len(notebook.cells) == 1 # it contains an empty cell | ||
|
||
# Make sure copied cells are deletable | ||
notebook.edit_cell(index=0, content=a) | ||
set_cell_metadata(notebook, 0, 'deletable', 'false') | ||
assert not cell_is_deletable(notebook, 0) | ||
notebook.to_command_mode() | ||
notebook.current_cell.send_keys('cv') | ||
assert len(notebook.cells) == 2 | ||
assert cell_is_deletable(notebook, 1) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 likedelete_cell
,cell_is_deletable
andset_cell_metadata
, do you think they also should be moved to theNotebook
class too?There was a problem hiding this comment.
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
andset_cell_metadata
can move to the notebook class, but let's leavedelete_cell
andcell_is_deletable
here for now. They can always be moved in another PR if we change our minds. :-)There was a problem hiding this comment.
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 :)