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

Revise python test and dependencies to sample applications #2080

Merged
merged 5 commits into from
Feb 10, 2022

Conversation

piponazo
Copy link
Collaborator

@piponazo piponazo commented Feb 8, 2022

As discussed today in our chat, there was some confusion about how to run the tests. It turns out we still had in the test folder some old Makefiles and bash scripts that were used to run the system-tests over the exiv2 application and some of the sample applications.

In this PR I did 2 things:

  • Remove the deprecated bash files & Makefile. All these tests were already ported to the python test suite.
  • Move some tests from tests/bugfixes to tests\bash_tests. Even though the files moved were indeed added while fixing some bugs, they depend on sample applications. We can discuss if that bash_tests folders is the right place for these files or whether we should create another category (something like tests\sample_tests). But the main idea is to keep the bugfixes folder just to test the exiv2 library and application.

@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #2080 (40a59ab) into main (1b90036) will decrease coverage by 0.21%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2080      +/-   ##
==========================================
- Coverage   62.08%   61.87%   -0.22%     
==========================================
  Files          96       96              
  Lines       19153    19059      -94     
  Branches     9798     9782      -16     
==========================================
- Hits        11891    11792      -99     
- Misses       4967     4984      +17     
+ Partials     2295     2283      -12     
Impacted Files Coverage Δ
src/safe_op.hpp 69.23% <0.00%> (-27.65%) ⬇️
include/exiv2/slice.hpp 69.64% <0.00%> (-21.89%) ⬇️
include/exiv2/error.hpp 60.71% <0.00%> (-4.81%) ⬇️
src/utils.cpp 38.46% <0.00%> (-1.93%) ⬇️
include/exiv2/value.hpp 82.68% <0.00%> (-0.56%) ⬇️
src/enforce.hpp 75.00% <0.00%> (+2.77%) ⬆️
src/getopt.cpp 67.30% <0.00%> (+5.76%) ⬆️
include/exiv2/metadatum.hpp 88.88% <0.00%> (+16.16%) ⬆️

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 1b90036...40a59ab. Read the comment docs.

kevinbackhouse
kevinbackhouse previously approved these changes Feb 9, 2022
CMakeLists.txt Show resolved Hide resolved
hassec
hassec previously approved these changes Feb 9, 2022
Copy link
Member

@hassec hassec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@piponazo piponazo merged commit 8d2f6ad into main Feb 10, 2022
@piponazo piponazo deleted the main_revisePythonTest branch February 10, 2022 07:07
@clanmills
Copy link
Collaborator

That's awesome work, @piponazo. README.md and CONTRIBUTING.md still discuss make tests. I'm bringing this up because that's why @mallman opened #2082. I discussed this with him and he closed the issue report. However the documents have not been reviewed and corrected.

574 rmills@rmillsmm-local:~/gnu/github/exiv2/main $ find . -name "*.md" | xargs grep 'make ' | grep -v 'cmake '
./exiv2.md:requirements of the target format. Some tags copied like this may not make 
./contrib/vs2019/README.md:consistent with all the choices that I make for the application (like
./contrib/vs2019/README.md:project. This make Visual Studio generate the correct dependency
./GIT_GUIDELINES.md:Commits should be atomic, i.e. they should make one self-contained
./GIT_GUIDELINES.md:- Changes made in different source which do not make sense without each other
./README.md:$ sudo make uninstall
./README.md:$ make                                             # build the code
./README.md:$ make CXX_FLAGS=-DEXIV2_DEBUG_MESSAGES
./README.md:$ sudo make install
./README.md:$ make tests
./README.md:| VERBOSE            | _**not set**_              | Makefile platforms | Instructs make to report its actions |
./README.md:for i in base-devel git coreutils dos2unix tar diffutils make                     \
./CONTRIBUTING.md:6. Now, make your change(s), add tests for your changes, and commit each change:
./CONTRIBUTING.md:        $ make tests         # Integration tests
./doc/readme-pvs-studio.md:The PVS username & key are configured as secrets in the security settings of the project. Then we make use of such
./fuzz/README.md:make -j $(nproc)
575 rmills@rmillsmm-local:~/gnu/github/exiv2/main $ 

@piponazo
Copy link
Collaborator Author

Thanks @clanmills for letting me know. I'll go through the different documents and try to fix all these occurrences

@clanmills
Copy link
Collaborator

Good stuff, @piponazo. @kmilos and I agreed that all documentation should always discuss build and testing using cmake and conan. We should avoid discussing "legacy" tools such as make. So the preferred way to build is:

$ cd <exiv2dir>
$ mkdir build 
$ cd build
$ cmake ..
$ cmake --build .
$ ctest
$ sudo make install

I think Milos may prefer to build in a totally different location. I can't remember exactly how to run ctest in a totally different directory. The preferred build/test command are something like:

$ cd <exiv2dir>
$ mkdir /tmp/exiv2-build
$ cmake . --B /tmp/exiv2-build   [-DCMAKE_BUILD_TYPE=RELEASE]
$ cmake --build /tmp/exiv2-build [--target target] [--config Release]
$ ctest --build-run-dir /tmp/exiv2-build  # This is wrong.  I don't know how to do this!
$ sudo cmake --build /tmp/exiv2-build --target install

@kmilos. Can you comment please?

@kmilos
Copy link
Collaborator

kmilos commented Feb 10, 2022

I don't think I was doing anything different to what CI scripts do, and there we run something like ctest --test-dir /tmp/exiv2-build

Btw, you no longer need mkdir /tmp/exiv2-build, as cmake -B /tmp/exiv2-build will create it for you. You might want to rm -rf it before or after the whole process though to have a clean slate...

@piponazo
Copy link
Collaborator Author

@clanmills You can find some examples of how to build from other directories in the CI pipelines under exiv2/.github/workflows.

In my opinion, we should prefer to document things as in your first code snippet. It makes the documentation less verbose and I think it is the approach most of users will use when working locally.

The second approach is specially useful for CI pipelines. One can sacrifice there some more characters to be more explicit about what are the source_code & build directories, without needing to move across directories.

@clanmills
Copy link
Collaborator

@kmilos. Thanks for your input. That's great.

And thank you to @piponazo for his efforts on this issue concerning python. I think we should keep the names "bashTests" and "bugfixTests" because change usually leads to confusion!

I'll deal with CONTRIBUTING.md and README.md at the weekend to service: #2082. When I open the PR, I'll invite you both to review.

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.

5 participants