Skip to content

Add missing tests to the dis module #103804

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

Closed
jkchandalia opened this issue Apr 24, 2023 · 4 comments · Fixed by #103806 or #103901
Closed

Add missing tests to the dis module #103804

jkchandalia opened this issue Apr 24, 2023 · 4 comments · Fixed by #103806 or #103901
Labels
tests Tests in the Lib/test dir

Comments

@jkchandalia
Copy link
Contributor

jkchandalia commented Apr 24, 2023

Improve test coverage for the dis module i.e cpython/lib/dis.py, tests are in cpython/Lib/tests/test_dis.py. Missing tests include:

  • dis.disco
  • dis.findlinestarts

Linked PRs

@nanjekyejoannah
Copy link
Contributor

nanjekyejoannah commented Apr 26, 2023

@jkchandalia can you discuss your motivation for dis.disco being covered before we close this? I didnt follow your email very well. I am abit busy until end of week, so @iritkatriel can follow up if she has time before then.

@jkchandalia
Copy link
Contributor Author

@nanjekyejoannah I had a look at dis.disco which is the other function we mentioned in the issue. It looks like disco = disassemble and only exists only for backward compatibility which is what I see here:

disco = disassemble # XXX For backwards compatibility

Because dis.disassemble appears to be thoroughly tested, I was wondering if we don't need to write tests for disco explicitly?

@iritkatriel
Copy link
Member

disco is documented: https://docs.python.org/3/library/dis.html#dis.disco
so strictly speaking it should be tested.

Perhaps it should also be deprecated, I don't know the history.

@jkchandalia
Copy link
Contributor Author

@iritkatriel @nanjekyejoannah it looks like dis.disco was introduced in the initial python commit in 1990 by Guido: 217a5fa

I'm assuming it's been kept around for backwards compatibility and may not yet be a good idea to deprecate but I defer to your judgement. I've opened a PR with a test for dis.disco assuming we want to keep this around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir
Projects
None yet
3 participants