Fixed a bug where ipy notebooks were split across test groups #10
Conversation
src/pytest_split/plugin.py
Outdated
| def _is_ipy_notebook(node_id): | ||
| path = node_id.split("::")[0] | ||
| if path.endswith(".ipynb"): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
This helper function is for inferring whether a node_id is related to an IPython notebook or not.
src/pytest_split/plugin.py
Outdated
| # Modify start_idx and end_idx so they match start/end of an ipynb file | ||
| if _is_ipy_notebook(item_node_ids[start_idx]): | ||
| start_idx = _get_boundary_idx_for_ipynb(item_node_ids, start_idx, "start") | ||
| if _is_ipy_notebook(item_node_ids[end_idx - 1]): # since Python indexing is [a, b) | ||
| end_idx = _get_boundary_idx_for_ipynb(item_node_ids, end_idx, "end") |
There was a problem hiding this comment.
Here, I will first inspect whether the node_id associted with start_idx and end_idx are IPython notebooks or not, and if so, I modify the indices so a single IPython notebook is not broken into different test groups.
src/pytest_split/plugin.py
Outdated
| def _get_boundary_idx_for_ipynb(item_node_ids, idx, mode): | ||
| if mode not in ["start", "end"]: | ||
| raise Exception(f"Unsupported mode: {mode}") | ||
| ipynb_node_id = item_node_ids[idx] | ||
| fname = ipynb_node_id.split("::")[0] | ||
| # If node_id provided instead of idx | ||
| # idx = item_node_ids.index(ipynb_node_id) | ||
| same_notebook = True | ||
| while same_notebook: | ||
| if idx in [0, len(item_node_ids)]: | ||
| break | ||
| idx_next = idx - 1 if mode == "start" else idx + 1 | ||
| fname_next = item_node_ids[idx_next].split("::")[0] | ||
| if fname_next == fname: | ||
| idx = idx_next | ||
| else: | ||
| same_notebook = False | ||
| return idx if mode == "start" else idx + 1 # since Python indexing is [a, b) 8000 |
There was a problem hiding this comment.
This is a helper function for obtaining the start/end indices of an IPython notebook, given node_id corresponding to a single cell of that file.
|
Hello @ma-sadeghi! Great to hear that you're using Could you briefly explain what's your use case with iPython notebooks, tests, and pytest-split? If you can share some example notebook(s), even better. I'm just a bit skeptical whether a pytest plugin itself is the correct place for notebook support 🤔 I mean, it's quite a bit of added complexity to have around 25% of all the code just for IPython notebook support. Perhaps you could fork this and create e.g. |
|
Thanks for getting back to me. Let me back up. So, there's this plugin In our packages, we use a combination of unit tests and IPython notebooks for testing (unit tests just for testing and notebooks both for testing and tutorial for users). Since we have many of these, we want to parallelize the testing process. But, the issue is that in the first cell of the notebook, and use the value of in this simple example, in principle You can find all our notebooks here. The reason I made this PR to your package, rather than forking it is that, I assume |
|
@jerry-git I just pushed a major refactor of the code I added, now down to ~10% of the entire code. |
|
Thanks for the context and great use case for pytest-split 👍 Alright, let's get the support done then. |
| end_idx = len(items) | ||
|
|
||
| # Modify start_idx and end_idx so they match start/end of an ipynb file | ||
| if _is_ipy_notebook(item_node_ids[start_idx]): |
There was a problem hiding this comment.
Could you please add some test case for this whole notebook thing?
There was a problem hiding this comment.
Have a look at test_plugin.py. I think you should be able to create some simple test notebook with testdir.makefile and for the rest there should be good examples already in the existing tests.
There was a problem hiding this comment.
@jerry-git I added test_ipynb.py which is basically almost the same as test_plugin.py, but for notebooks. I tried merging it with test_plugin.py but it's my first time working with pytest fixtures, and I'm still trying to wrap my head around it! Also, for some reason, running pytest on test_plugin.py and test_ipynb.py individually works fine, but it fails when invoking it on both, i.e. via running simply pytest. Could you please take a look? Thanks.
…er than accepting mode arg
|
Sorry it took a while to have a chance to look at this. It looks like there's something fishy going on with nbval, here's a minimal repro for the root cause of the issue: The first one is successful, the second one fails. So, basically, if I just run the exact same suite twice with only |
Fixes #9. @jerry-git Could you please take a look and merge if you agree? Thanks!