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 tests to unittest #28

Merged
merged 4 commits into from
Mar 26, 2019

Conversation

christian-intra2net
Copy link
Contributor

Sofar, there was a simple tests.sh script for testing, that required msoffcrypto-tool was already installed.

Converted that script to a python unittest that performs the same action but can run without installing msoffcrypto-tool

Renamed dir tests to test because when building rpms/source tarballs using setup.py, "test" is the hard-coded name where tests are expected. Should be included that way

@christian-intra2net
Copy link
Contributor Author

I just realized a MANIFEST.in is required to include the test data into the distribution tarball ("python setup.py sdist"). That means that moving "tests" to "test" was unnecessary. I will therefore revert the first commit and instead add the test source code to the MANIFEST

@christian-intra2net christian-intra2net force-pushed the convert-tests-to-unittest branch 2 times, most recently from 4d8864f to ddf8397 Compare March 21, 2019 11:13
@codecov-io
Copy link

codecov-io commented Mar 21, 2019

Codecov Report

Merging #28 into master will increase coverage by 50.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #28       +/-   ##
===========================================
+ Coverage   36.68%   86.72%   +50.04%     
===========================================
  Files          13       13               
  Lines        1145     1145               
===========================================
+ Hits          420      993      +573     
+ Misses        725      152      -573
Impacted Files Coverage Δ
msoffcrypto/method/ecma376_standard.py 98.21% <0%> (+17.85%) ⬆️
msoffcrypto/format/ooxml.py 78.12% <0%> (+31.25%) ⬆️
msoffcrypto/method/ecma376_agile.py 90% <0%> (+36%) ⬆️
msoffcrypto/__init__.py 78.26% <0%> (+39.13%) ⬆️
msoffcrypto/format/xls97.py 87.5% <0%> (+48.12%) ⬆️
msoffcrypto/format/doc97.py 88.69% <0%> (+58.26%) ⬆️
msoffcrypto/method/rc4_cryptoapi.py 95.23% <0%> (+61.9%) ⬆️
msoffcrypto/format/ppt97.py 98.86% <0%> (+70.45%) ⬆️
msoffcrypto/format/common.py 100% <0%> (+77.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8689b89...f20cfad. Read the comment docs.

@nolze
Copy link
Owner

nolze commented Mar 22, 2019

Amazing! Thank you so much.

While the library usage of msoffcrpyo-tool is now nicely covered by your new tests, I intended to test the CLI usage of the tool using tests.sh.

That is, ideally there should be:

  1. a few illustrative unit tests with doctest (w.r.t. internal and library usage),
  2. comprehensive unit/integration tests for all sample documents with unittest (w.r.t. library usage, your contribution to the latter here),
  3. integration tests for all or some documents with a shell script (w.r.t. CLI usage).

So, my suggestion is to merge your PR preserving tests.sh. Concerning your point, I think that changing msoffcrypto-tool in tests.sh to python -m msoffcrypto we will no longer need to install it prior to testing.

@christian-intra2net
Copy link
Contributor Author

christian-intra2net commented Mar 25, 2019

Glad you like it :-)
I did think about creating a unittest that just runs tests.sh line by line using subprocess.check_output. Then decided that the way I implemented is easier and does almost the same.
But you're the boss, I will modify this branch

Required e.g. for tarballs created using python setup.py sdist (e.g. for pip)
to include tests and the readme file
This way can run nosetests before build and install
@nolze nolze merged commit 81b5b25 into nolze:master Mar 26, 2019
@nolze
Copy link
Owner

nolze commented Mar 26, 2019

Merged! I'll release a new version with this update soon.

@nolze nolze removed the in progress label Mar 26, 2019
@christian-intra2net
Copy link
Contributor Author

Cool :-) Thanks

@christian-intra2net christian-intra2net deleted the convert-tests-to-unittest branch April 25, 2019 16:30
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.

None yet

3 participants