search

cytoscape / ipycytoscape

A Cytoscape Jupyter widget
https://ipycytoscape.readthedocs.io/en/master/
BSD 3-Clause "New" or "Revised" License
269 stars 62 forks source link

To have a 100% test coverage on ipycytoscape #128

Open marimeireles opened 4 years ago

marimeireles commented 4 years ago

From @ianhi comment's:

Still needed:

If you're adding one of these tests please reference this issue.

ianhi commented 4 years ago

will require using the mocked comms to fake a message.

This was just to note that these mocks already exist from the cookiecutter.

maybe use karma-typescript (I think it came with the js cookie cutter)

These have been updated a bit since ipycytoscape was created in order to get them pass. Should be based on the content here: https://github.com/jupyter-widgets/widget-ts-cookiecutter/tree/master/%7B%7Bcookiecutter.github_project_name%7D%7D/tests

ianhi commented 4 years ago

pre 100% I think we should focus on having tests for the methods to create the graph as these have definitely been the source of pretty much all new bugs. Networkx had the most bugs so made those tests first. With first set of tests the coverage looks like this: https://codecov.io/gh/quantstack/ipycytoscape/src/0079bf2768036df4435c7d10e3927d9e7de19b34/ipycytoscape/cytoscape.py

So we should also add:

marimeireles commented 4 years ago

I think we covered these:

json graph creation
df graph creation
tests for CytoscapeWidget methods

Right @ianhi ? Also, do you know any example we can copy for

Tests of adding interaction callbacks

ianhi commented 4 years ago

I think we covered these:

I don't thnk we have. We've just added more complete tests of the possibilities with networkx https://github.com/QuantStack/ipycytoscape/blob/master/ipycytoscape/tests/test_graph_creation.py

json is probably the easiest thing for someone to get started on.

Also, do you know any example we can copy for

Tests of adding interaction callbacks

Not off the top of my head. I think we would need to make Mock events and feed them in. But if anything this would make more sense on the typescript side. Maybe this is one to leave until there is a better testing framework for jupyter.

SvenSchiffner commented 3 years ago

So we should also add:

* [ ]  test `add_node`. `add_edge`, `remove_node`, `remove_edge`, `remove_edge_by_id`

I'm not very familiar with automated testing. Is it necessary to test remove_edge and remove_edge_by_id? Basically remove_edge_by_id calls remove_edge. So if remove_edge_by_id works like intended this should also be valid for remove_edge, or miss I something?

(I try to write some tests for this at the moment.)

joseberlines commented 3 years ago

what do you mean by "df graph creation"?

SvenSchiffner commented 3 years ago

Dataframe graph creation - Create a graph from a pandas dataframe

joseberlines commented 3 years ago

Ok, I am on that one right now. I actually would have some questions. Should I open an issue on questions about this implementation? How is it possible to share code to be commented/discussed over before pulling? Actually it is perfectly possible

ianhi commented 3 years ago

Is it necessary to test remove_edge and remove_edge_by_id? Basically remove_edge_by_id calls remove_edge. So if remove_edge_by_id works like intended this should also be valid for remove_edge, or miss I something?

I think everything you said here is correct, but it can still be nice to test both as that makes easier to figure out what went wrong. So perfect world do both, but don't let the perfect be the enemy of the good and either would be an improvement over the current situation.

ianhi commented 3 years ago

I actually would have some questions. Should I open an issue on questions about this implementation? How is it possible to share code to be commented/discussed over before pulling? Actually it is perfectly possible

Feel free to open PRs to get early feedback if you'd like. Otherwise for tests posting quesitons in this issue is probably a good move.

SvenSchiffner commented 3 years ago

pre 100% I think we should focus on having tests for the methods to create the graph as these have definitely been the source of pretty much all new bugs. Networkx had the most bugs so made those tests first. With first set of tests the coverage looks like this: https://codecov.io/gh/quantstack/ipycytoscape/src/0079bf2768036df4435c7d10e3927d9e7de19b34/ipycytoscape/cytoscape.py

So we should also add:

* [ ]  test `add_node`. `add_edge`, `remove_node`, `remove_edge`, `remove_edge_by_id`

225

marimeireles commented 3 years ago

pre 100% I think we should focus on having tests for the methods to create the graph as these have definitely been the source of pretty much all new bugs. Networkx had the most bugs so made those tests first. With first set of tests the coverage looks like this: https://codecov.io/gh/quantstack/ipycytoscape/src/0079bf2768036df4435c7d10e3927d9e7de19b34/ipycytoscape/cytoscape.py So we should also add:

* [ ]  test `add_node`. `add_edge`, `remove_node`, `remove_edge`, `remove_edge_by_id`

225

:cherry_blossom: