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

Rewrite re2c tests runner in Python #360

Merged
merged 1 commit into from
Jun 19, 2021

Conversation

sergeyklay
Copy link
Collaborator

@sergeyklay sergeyklay commented Jun 16, 2021

This pull request introduces a completely rewritten test runner in Python.

The main goals of this change:

  • Provide a OS-independent way to run tests (at least using CMake)
  • Getting rid of dependence on Unix utilities in tests (grep, cat, tr, etc.)
  • Flexibility
  • Ease of maintenance

@sergeyklay sergeyklay added the wip label Jun 16, 2021
@sergeyklay sergeyklay self-assigned this Jun 16, 2021
@sergeyklay sergeyklay linked an issue Jun 16, 2021 that may be closed by this pull request
@sergeyklay
Copy link
Collaborator Author

There is no /home/skvadrik/devel/50/re2c/include among them, and no suitable relative path (considering that the working directory is /home/skvadrik/devel/50/re2c/test_210616222213/, -I include means a copy of /home/skvadrik/devel/50/re2c/test/include, not /home/skvadrik/devel/50/re2c/include.

If I patch the command and add -I ../include then
{c,go}/unicode_identifier.re do not fail anymore and I'm down to two
failures:

Yes you are right. This is exactly what I decided at the very beginning. However, this doesn't work for some reason:

Right now I tried:

diff --git a/run_tests.py b/run_tests.py
index 9c93e4a9..2551e9ee 100644
--- a/run_tests.py
+++ b/run_tests.py
@@ -539,7 +539,8 @@ def init_process(base_path, skeleton=False, keep_temp_files=False,
     # abshere('@top_srcdir@', 'include')
     #   vs
     # abshere('..', '@top_srcdir@', 'include')
-    _ctx.incpaths.append(f"-I {abshere(base_path, 'include')}")
+    _ctx.incpaths.append("-I /Users/egrep/work/re2c/include")
+    _ctx.incpaths.append("-I ../include")
 
     # Set tools paths
     _ctx.diff = which('diff')[0]

And output is still the same:

$ python run_tests.py --keep-temp-files

-----------------
All:         1615
Ran:         1615
Passed:      1611
Soft errors: 0
Hard errors: 4
-----------------
FAILED

@sergeyklay
Copy link
Collaborator Author

@skvadrik Can you please commit your patch with the fix of the two tests? I may be missing something

@skvadrik
Copy link
Owner

Thanks a lot for working on this!

One thing off the top of my head is that skeleton test failures can be deceptive: if re2c fails with an error, it is counted as a "soft error" because we have some tests that deliberately fail. Currently I see 222 soft failures in run_tests.py and 221 for run_tests.sh. The one extra error will likely go away after fixing non-skeleton tests, but the whole "soft error" concept is very weak and I added it as a temporary hack to monitor how many tests actually run and how many fail before that.

@skvadrik
Copy link
Owner

@skvadrik Can you please commit your patch with the fix of the two tests? I may be missing something

I think I know what it is, it should be two separate arguments -I and ./include, not -I ../include. At least I tried that change and the errors dropped from 4 to 2.

@sergeyklay
Copy link
Collaborator Author

Currently I see 222 soft failures in run_tests.py and 221 for run_tests.sh.

Yeah, I saw this yesterday, but for now I decided to focus on non-skeleton tests.

@sergeyklay
Copy link
Collaborator Author

I think I know what it is, it should be two separate arguments -I and ./include, not -I ../include. At least I tried that change and the errors dropped from 4 to 2.

Yay! I spent about 8 hours on this! You're Chuck Norris :)

@skvadrik
Copy link
Owner

Yay! I spent about 8 hours on this! You're Chuck Norris :)

I'm used to spending 8h a day on things like this (most of my working day really), so no wonder I could spot it. :D

@sergeyklay
Copy link
Collaborator Author

sergeyklay commented Jun 16, 2021

I think I know what it is, it should be two separate arguments -I and ./include, not -I ../include. At least I tried that change and the errors dropped from 4 to 2.

Yay!

diff --git a/run_tests.py b/run_tests.py
index 8b84fa3d..ecd8ae57 100644
--- a/run_tests.py
+++ b/run_tests.py
@@ -532,7 +532,8 @@ def init_process(base_path, skeleton=False, keep_temp_files=False,
     # Set include paths, relative to build directory
     cwd = os.getcwd()
     os.chdir(_ctx.base_path)
-    _ctx.incpaths = [f'-I {d}' for d in Path('.').rglob('*') if os.path.isdir(d)]
+    paths = Path('.').rglob('*')
+    _ctx.incpaths = sum([['-I', str(d)] for d in paths if os.path.isdir(d)], [])
     os.chdir(cwd)
 
     # TODO: Fix this to sort out with absolute and relative paths + @top_srcdir@
-----------------
All:         1615
Ran:         1615
Passed:      1615
Soft errors: 0
Hard errors: 0
-----------------

Explanation

list1 = ['111', '222', '333']
sum([['-I', item] for item in list1], [])

# Out: ['-I', '111', '-I', '222', '-I', '333']

@sergeyklay
Copy link
Collaborator Author

@skvadrik I updated the description of the PR. All tests are passed now.

@skvadrik
Copy link
Owner

@skvadrik I updated the description of the PR. All tests are passed now.

\o/

@sergeyklay sergeyklay changed the title WIP: re2c test runner in Python Rewrite re2c test runner in Python Jun 17, 2021
@sergeyklay sergeyklay marked this pull request as ready for review June 17, 2021 08:26
@sergeyklay sergeyklay removed the wip label Jun 17, 2021
@sergeyklay sergeyklay requested a review from skvadrik June 17, 2021 08:27
@sergeyklay
Copy link
Collaborator Author

It is ready to review

@skvadrik
Copy link
Owner

It is ready to review

Awesome, thanks! I may be unable to review it today, but I'll do it at the first opportunity.

@sergeyklay
Copy link
Collaborator Author

There are some issues with external tools on Windows. I'll try to sort out in next PRs to not mix changes in this one

@sergeyklay sergeyklay force-pushed the feature/py-test-runner branch 2 times, most recently from 801f248 to d045ef2 Compare June 17, 2021 22:38
@sergeyklay
Copy link
Collaborator Author

Some good (or bad) news: I managed to run tests runner under Windows. And I see half of the tests are failed. On the one hand, this is bad news, but on the other hand, we did not have the opportunity to run native compilation (not Mingw) and tests under WIndows until this moment. So now we see the real picture

@skvadrik
Copy link
Owner

Some good (or bad) news: I managed to run tests runner under Windows. And I see half of the tests are failed.

Can you start a new bug and post detail how the tests failed? E.g. upload the temporary test directory as an archive.

@sergeyklay
Copy link
Collaborator Author

Can you start a new bug and post detail how the tests failed? E.g. upload the temporary test directory as an archive.

Sure. Let me some time to sort out with unstable test runner under the Windows (some regex compilation issues under the loops)

@sergeyklay
Copy link
Collaborator Author

sergeyklay commented Jun 18, 2021

Some good (or bad) news: I managed to run tests runner under Windows. And I see half of the tests are failed. On the one hand, this is bad news, but on the other hand, we did not have the opportunity to run native compilation (not Mingw) and tests under WIndows until this moment. So now we see the real picture

I fixed tests runner to run under the Windows. There were some funny bugs like Windows directory separator \ interpreted as a regex escape command, some Cmake issues related to build under the Windows and so on. And at the moment we have the following at GitHub actions:

Running in 2 job(s) 
----------------- 
All: 1615 
Ran: 1615 
Passed: 264 
Soft errors: 0
Hard errors: 1351 
----------------- 
FAILED

I'll open a new PR with all necessary changes and show you all the remaining issues. But lets do it after this one will be merged so that I can rebase onto the proper branch and open a PR.

@sergeyklay sergeyklay changed the title Rewrite re2c test runner in Python Rewrite re2c tests runner in Python Jun 18, 2021
Copy link
Owner

@skvadrik skvadrik left a comment

Choose a reason for hiding this comment

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

Thanks again for doing this work! I don't know python very well, and to me the code looks nice and clean (much better than bash!). There is a couple of things to fix, but largely it just works. \o/

CMakeLists.txt Show resolved Hide resolved
run_tests.py.in Outdated Show resolved Hide resolved
@skvadrik
Copy link
Owner

skvadrik commented Jun 18, 2021

By the way, the new script is already ~2x faster than the old one (maybe more). :)

run_tests.py.in Outdated Show resolved Hide resolved
run_tests.py.in Show resolved Hide resolved
This pull request introduces a completely rewritten test runner
in Python.

The main goals of this change:

- Provide a OS-independent way to run tests (at least using CMake)
- Getting rid of dependence on Unix utilities in tests
  (grep, cat, tr, etc.)
- Flexibility
- Ease of maintenance
@sergeyklay
Copy link
Collaborator Author

@skvadrik It is ready to merge now.

@skvadrik skvadrik merged commit 9c99811 into skvadrik:master Jun 19, 2021
@skvadrik
Copy link
Owner

Great, I have just merged it! \o/

@sergeyklay
Copy link
Collaborator Author

sergeyklay commented Jun 19, 2021

Great, I have just merged it! \o/

Yay! I'm glad this finally happened :)

@sergeyklay sergeyklay deleted the feature/py-test-runner branch June 19, 2021 17:44
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.

3 participants