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 __main__ tests into unit tests #537

Closed
wants to merge 8 commits into from

Conversation

infinnovation-dev
Copy link

Several modules have main sections containing tests. These have been converted to proper unit tests. runall.sh has been updated accordingly.

Some of the original only printed results, so did not detect regressions when run via CI. In some cases (e.g. structure), there are additional checks on the binary form as opposed to just round-trip.

If attempting to merge these changes into python36, some errors are seen in test_krb5_crypto and test_dcerpc_v5_ndr; it's not clear whether the tests or the tested modules need adjusting. Also, test_dns shows some deprecation warnings.

I've left several separate commits in this PR at the moment, in the expectation that there may be some edit / rebase / squash actions required before it is ready to merge.

@asolino
Copy link
Collaborator

asolino commented Dec 6, 2018

Hey @infinnovation-dev

Thanks a lot for taking the time to better test the library. Much appreciated. There's one important thing that might be a headache to you, but something needed. Looks like you've been working on the master branch, and that is the reason why you have tests failing on python36.

As you might have noticed, I tagged a new release yesterday. That is the latest version that is Python 2.x only. I'll be merging python36 into master in the next few days, so we can keep working from now on both on Python 3.x and Python 2.x. In fact, python36 has all the test cases both working on 3.x and 2.x, something that doesn't happen currently in master (especially the SMB/DCE tests).

What this all means is that I would need you to do the work you did in the python36 branch instead of master. Hopefully this is something you can do.

@infinnovation-dev
Copy link
Author

OK, I had seen that you were regularly merging master into python36, so my thought was that by preparing a PR against master, both branches should benefit from any improved CI goodness. But if you're about to merge the other way, that's moot. I had been cherry-picking my changes onto a python36-based branch to compare, as I went along, so it should be easy enough for me to make a new PR against python36. As I said, a couple of tests may fail; perhaps someone who understands more of the internals can fix those. (I see you still have allow_failures set for 3.6.)

@asolino
Copy link
Collaborator

asolino commented Dec 6, 2018

Indeed I usually merge master into python36. That will help merge python36 into master way easier (it should be transparent actually). However, python36 contains many more features related to Python 3.x support that are not in master. If you base your work on python36 branch the result should be that the test cases should work both on 2.X and 3.X.

For example, let's take your test_krb5_crypto, you took the __main__ section from master's krb5/crypto.py file. However, if you take a look at the same file in python36 you will notice there are some differences, in fact, those are the changes to make the code to work in 3.X. Long story short (and that's why I said it is a headache for you), you will need to do the same work you did but basing your code in python36 branch.

Hope I made myself clear.

@infinnovation-dev
Copy link
Author

You're right of course; I wasn't thinking straight. If the main test works in python36 then it ought to be able to pass when converted to a unit test

@asolino
Copy link
Collaborator

asolino commented Dec 6, 2018

Thanks again for your help @infinnovation-dev 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants