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

Remove test data from repository and use data from zim-testing-suite. #538

Merged
merged 6 commits into from
Apr 28, 2021

Conversation

mgautierfr
Copy link
Collaborator

This PR remove the data from the repository and download the data from zim-testing-suite.

It should be considered as a preparatory work of #535.
It replace #531 answering comments addressed:

  • Put the test data version somewhere in the code to know which data to use with which version of the code.
  • Simplify the downloading of the data (now repository contain a scrip to do it and can be run through meson).
  • User can download the data and use the downloaded data.

@legoktm On packaging side, you can use the provided script to download the data or make the archive a second source and just configure meson correctly.

For now, this PR works only if test_data_dir option is not used (tests look in test/data directory in build dir).
This PR must be considered with #535 where test are using data pointed by the env var.
The two PR will be merge together to avoid to add the new zim files to the repository. But I've created a separated PR to concentrate on the download process as we already agreed on the new tests themselves.

@codecov
Copy link

codecov bot commented Apr 20, 2021

Codecov Report

Merging #538 (f19eaec) into master (02d451f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #538   +/-   ##
=======================================
  Coverage   79.30%   79.30%           
=======================================
  Files          91       91           
  Lines        3744     3744           
  Branches     1701     1701           
=======================================
  Hits         2969     2969           
  Misses        774      774           
  Partials        1        1           

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 02d451f...f19eaec. Read the comment docs.

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

README.md must be updated

@mgautierfr
Copy link
Collaborator Author

README.md must be updated

Done.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mgautierfr
Copy link
Collaborator Author

@legoktm Is it ok for you ?
I don't know how to patch debian/rules to add the ninja download_test_data during the build.
(I'm not even sure we should as the build step is probably offline.)

@legoktm
Copy link
Member

legoktm commented Apr 23, 2021

@legoktm Is it ok for you ?

As you had suggested in #531 (comment), can we have an environment variable that causes it to skip these tests? That can be set in debian/rules and then fixing the packaging no longer blocks you.

I don't know how to patch debian/rules to add the ninja download_test_data during the build.
(I'm not even sure we should as the build step is probably offline.)

Yes, building the Debian package shouldn't require any Internet dependencies. We (I really) should add the packaging configuration to zim-testing-suite so it's in our PPA and libzim can depend on it like a normal apt dependency. I can start working on that over the weekend.

@kelson42
Copy link
Contributor

@legoktm Any news about a fix of the deb package CI?

The script can be run directly but can (and will) be also called by meson.
The target is not run by default.
It allow user to download the data itself if he wish.
To download the data using meson, one must run `ninja download_test_data`
once configured.

The path to the data dir must be passed to meson using the
`test_data_dir` option (`-Dtest_data_dir=/foo/test_data`).
If the option is not passed, default to a subdirectory inside the build
directory.
Downloading external files to run tests may not be optimal depending of
the situation.
This allow to be able to still compile/test libzim without downloading
the test files.
@mgautierfr
Copy link
Collaborator Author

@legoktm last commit add a option to deactivate tests using external test data.

We need to pass the option -Dtest_data_dir=none to meson.
If you tell we how to pass this option to meson in the debian/rules I can do the patch myself and we will be good.

Skips zim-testing-data tests until we package that.
@legoktm
Copy link
Member

legoktm commented Apr 27, 2021

@legoktm last commit add a option to deactivate tests using external test data.

We need to pass the option -Dtest_data_dir=none to meson.
If you tell we how to pass this option to meson in the debian/rules I can do the patch myself and we will be good.

I had to look it up myself :) pushed the fix, which also makes it obvious how to add more config flags in the future.

@kelson42
Copy link
Contributor

There is still two codefactor issues.

@mgautierfr
Copy link
Collaborator Author

There is still two codefactor issues.

I consider them as two false positives.

@mgautierfr mgautierfr merged commit 75fcc61 into master Apr 28, 2021
@mgautierfr mgautierfr deleted the no_test_data2 branch April 28, 2021 12:56
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.

4 participants