-
Notifications
You must be signed in to change notification settings - Fork 393
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
improve testing support and the CI #708
improve testing support and the CI #708
Conversation
Can you split out the utils2.c "/tmp" rewriting for Apple from the rest of this PR? That's the only part of your PR for which I may have semi-intelligent input. For that, please add comments at the top of genPathname() for the Apple rewrites you are adding. In one of your cmake changes, at line 41 I saw "sudo sudo ..." I see that you're implementing the test suite in cmake. It also looks like you're allowing some degree of parallelism. I am vary interested in the results. In linux, if you allow too much parallelism the tests will fail due to race conditions between the run that generates the golden data and the one that compares with it; i.e., the generation run has not completed and the compare run overtakes it. |
Just to be clear on the part about the race condition in the tests in linux: this is when reg_wrapper.sh is used with autotools, and you invoke the test run with |
86bab52
to
59dba92
Compare
Yep, it's a separate commit, see #709.
Amended.
My analysis of the issue is that a test generate run can interfere with another test compare run. |
I can run the testsuite successfully on Linux with |
59dba92
to
0117908
Compare
@benoit-pierre Dan |
0117908
to
c6e04f0
Compare
When a new workflow run is started on the same branch/tag, abort the previous ones (no sense in wasting CI resources).
- use bash shell for convenience and better error handling - split leptonica setup/build/install step for easier inspection
Disable LZMA support for now (unresolved symbols at link time).
By isolating each test temporary directory.
c6e04f0
to
20f725a
Compare
This set of commits is a major advance in testing functionality of the library and programs on different platforms. (1) Commit (2) Commit Dan |
There already exists a convention for rewriting the directory for temporary data by using the environment variable One problem which must be solved for parallel tests is that some Leptonica functions use "temporary files" with filenames which cause a conflict if more than a single process uses such functions. That problem is a general problem. If it is solved for the tests by using many different temporary directories, it still remains for all other use cases. With the current pull request two different parallel runs of Another problem of the tests is that some tests depend on others, so the order in which tests are run is important. And finally a complete fix for parallel tests must also handle the autoconf builds (= fix Unrelated: @DanBloomberg, why are those files in /tmp/lept not real temporary files which get removed after they were used? |
|
I just read the commit message for c72049d. That reasoning is still valid, not only for UNIX / Linux, but also for other operating systems like macOS and Windows. So there is a Tesseract bug caused by Leptonica's rewriting of /tmp for Windows since a long time, and we now have introduced the same bug for macOS which is more severe because on macOS /tmp always exists. I'm afraid I need more time to find a good solution for that mess. |
Are you saying there is a current bug in Tesseract on Windows due to rewriting, or in old versions from more than 5 years ago? As you can see from that commit and others around the same time when Fixing Tesseract is top priority -- whatever you come up with is OK with me. |
fi | ||
|
||
if [ -z "${LEPT_TMPDIR}" ]; then | ||
export LEPT_TMPDIR="${PWD}/${TEST_NAME}.tmp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the environment variable set conditionally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reg_wrapper.sh uses /bin/sh according to line 1, but not every /bin/sh supports the export syntax which is used here.
I have no objection to removing the newly-added |
Testing:
/tmp/…
paths accesses to the same location (similarly to was is done on Windows)CI:
cmake_win64
workflow: the libtiff build was failing (disable LZMA support for now), the version of giflib being built was 5.1.2 (broken)cmake_win64
workflow for all branchescmake_…
workflows