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 dualmode_insertcell.js #3508

Merged

Conversation

@sheshtawy
Copy link
Contributor

@sheshtawy sheshtawy commented Apr 6, 2018

This PR is addressing issue #3335 in order to convert js tests to selenium. It contains the conversion of the file notebook/tests/notebook/dualmode_insertcell.js

@takluyver
Copy link
Member

@takluyver takluyver commented Apr 8, 2018

👍 are you waiting for anything from us?

Loading

@sheshtawy
Copy link
Contributor Author

@sheshtawy sheshtawy commented Apr 9, 2018

No, I'm sorry I just got busy with a little task at work. I'll start working on notebook once I'm done with it I promise it will be soon). Thanks for checking in :)

Loading

@sheshtawy
Copy link
Contributor Author

@sheshtawy sheshtawy commented Apr 14, 2018

@takluyver I have a question
For the code block below (it's from from dualmode_cellinsert.js) I don't understand why it exists, it kinda feels repetitive. Can you help me understand why do we have it?

this.then(function () {
        this.select_cell(2);
        this.trigger_keydown('y'); // switch it to code for the next test
        this.test.assertEquals(this.get_cell(2).cell_type, 'code', 'test cell is code');
        this.trigger_keydown('b'); // new cell below
        this.test.assertEquals(this.get_cell(3).cell_type, 'code', 'b; inserts a code cell below code cell');
        this.trigger_keydown('a'); // new cell above
        this.test.assertEquals(this.get_cell(3).cell_type, 'code', 'a; inserts a code cell above code cell');
    });
    
    this.then(function () {
        this.set_cell_text(1, 'cell1');
        this.select_cell(1);
        this.select_cell(2, false);
        this.trigger_keydown('a');
        this.test.assertEquals(this.get_cell_text(1), '', 'a; New cell 1 text is empty');
        this.test.assertEquals(this.get_cell_text(2), 'cell1', 'a; Cell 2 text is old cell 1');
        
        this.set_cell_text(1, 'cell1');
        this.set_cell_text(2, 'cell2');
        this.set_cell_text(3, 'cell3');
        this.select_cell(1);
        this.select_cell(2, false);
        this.trigger_keydown('b');
        this.test.assertEquals(this.get_cell_text(1), 'cell1', 'b; Cell 1 remains');
        this.test.assertEquals(this.get_cell_text(2), 'cell2', 'b; Cell 2 remains');
        this.test.assertEquals(this.get_cell_text(3), '', 'b; Cell 3 is new');
        this.test.assertEquals(this.get_cell_text(4), 'cell3', 'b; Cell 4 text is old cell 3');
    });

Loading

@takluyver
Copy link
Member

@takluyver takluyver commented Apr 14, 2018

The first chunk is a simple test for inserting a cell above and below the current one, though it doesn't look like it's a very good test.

The second part is testing with multiple cells selected, to check that the new cell is added in the right place - before the first selected cell, or after the last selected cell.

Loading

@sheshtawy
Copy link
Contributor Author

@sheshtawy sheshtawy commented Apr 18, 2018

So just to make sure I understand the purpose of the test correctly and not just copy it from js to python:

  • The main purpose of this test case is to test the "insert" functionality in both edit and command modes? Or is it just about testing out insertion in different scenarios? @takluyver

Loading

@takluyver
Copy link
Member

@takluyver takluyver commented Apr 18, 2018

I think it's only dealing with command mode. IIRC, all the dualmode_*.js test files were added when we introduced the distinction between edit mode and command mode, to test the behaviour and the new shortcuts.

Loading

@sheshtawy sheshtawy changed the title [WIP] Convert dualmode_insertcell.js Convert dualmode_insertcell.js Apr 26, 2018
notebook.current_cell.send_keys("a")
assert notebook.get_cell_type(3) == "markdown"
notebook.current_cell.send_keys("b")
assert notebook.get_cell_type(4) == "markdown"
Copy link
Member

@takluyver takluyver Apr 26, 2018

Choose a reason for hiding this comment

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

I think the thing this is trying to test no longer works like that. We used to insert a new cell of the same type as the one you had selected, but we eventually decided it was easier to follow if it was always a code cell that was inserted. The test passes, but I think that's two accidents cancelling each other out - the cell you converted to markdown becomes number 3, then number 4, as you insert other cells.

Loading

Copy link
Contributor Author

@sheshtawy sheshtawy Apr 26, 2018

Choose a reason for hiding this comment

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

Oh .. okay. I will remove those steps and assertions then. Do you have any other comments?

Loading

Copy link
Contributor Author

@sheshtawy sheshtawy Apr 26, 2018

Choose a reason for hiding this comment

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

I was actually confused about that. I tried to do the test manually and I wasn't able to insert markdown cells. It all makes sense now

Loading

Copy link
Contributor Author

@sheshtawy sheshtawy Apr 26, 2018

Choose a reason for hiding this comment

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

Also, I deeply apologize for the delay in submitting this PR. I will do my best to make sure this doesn't happen again and I hope this doesn't affect your idea of how committed I am to the team and the project.

Loading

Copy link
Member

@takluyver takluyver Apr 26, 2018

Choose a reason for hiding this comment

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

No need to apologise! Everyone gets busy at times, and no-one expects volunteer contributors to do things by any set deadline. We might ping you on unfinished pull requests in case you've forgotten about it, or in case you're waiting for us to answer something, but if you don't have time to work on it, that's fine. If it's urgent, someone else can pick it up and finish it, and if not, it can wait. :-)

Loading

@takluyver
Copy link
Member

@takluyver takluyver commented Apr 26, 2018

I'm a bit too tired for a detailed review now, but I think it's still missing the tests from the JS version which check inserting a cell when multiple cells are already selected?

Once you're happy that you've made Python equivalents of the JS tests, feel free to go ahead and delete the JS file in this PR.

Loading

assert notebook.get_cell_contents(1) == ""
assert notebook.get_cell_contents(2) == "cell1"
assert notebook.get_cell_contents(3) == "cell2"
assert notebook.get_cell_contents(4) == ""
Copy link
Member

@takluyver takluyver Apr 28, 2018

Choose a reason for hiding this comment

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

For something like this, I think it would be an improvement to get all the cells at once:

assert notebook.get_cells_contents()[:4] == ["", "cell1", "cell2", ""]

If the test fails, I think the comparison of the lists will give a more informative error to see what has gone wrong. It's also shorter and I guess it's more efficient (less requests to the browser).

Loading

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

@takluyver takluyver commented Apr 28, 2018

Thanks!

Loading

@takluyver takluyver merged commit 2aca6f9 into jupyter:master Apr 28, 2018
4 checks passed
Loading
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 1, 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