-
Notifications
You must be signed in to change notification settings - Fork 72
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
build: remove the dependency on GNU Parallel for running unit tests #243
Conversation
@rtpro I think that we should add gtest-parallel as a submodule to our repo, instead of what I did in this PR, which is take the files out of their repo and do the modifications we needed. That way we can be up to date with changes they make, as well as make this diff smaller, to represent only what I have added. Thoughts? |
ec99454
to
003cc27
Compare
30f8118
to
96e245b
Compare
@ayulas please test |
im not sure what i need to see. the make check on my mac takes loooong time. i dont see any changes regarding the parallel |
Is it the build process that is taking long or is it the part where the tests actually begin to run? Can you please try to isolate the two and let me know? |
the build is always very fast on mac... the tests running is almost an hour.... also in main when i run there is on the left side UI passed / failed and what tests are running. in your branch only how many tests to run and how much was completed |
Regarding feedback on tests: if a test fails it'll show you that in the summary. |
its not an hardware issue since my mac is new and fast . |
The changes in this PR are not specific to MacOS. What you are referring to is me saying I need to make modifications to this PR so that it’ll work on MacOS as well, because when I first opened the PR it only worked on Linux. The purpose of this PR is written in the title and the description, hence why I need Udi to test it as well. |
And on your branch:
|
@AmnonHanuhov - Is that what you have wanted me to do? |
@RoyBenMoshe Can you also please test this on your Mac and share the results? |
7588a74
to
751aba3
Compare
I am trying to view the generated log files of the parallel run. I am notified that they may be binary files: |
I see that there is a folder containing all of the logs for the passed tests. |
Is there a timeout on tests that get stuck? |
I ran the "make check" on both main and this branch on my M1 Mac running Monterey.
Computers / CPU cores / Max jobs to run Computer:jobs running/jobs completed/%of started jobs/Average seconds to complete
Note that the time was substantially longer in the new code, but this might have been that the tests which timed out actually completed, or it could have been part of the rest of the "make check" pieces. Also note that the new code does not support "make watch-log" -- just prints and error and "waits" |
A folder for the failed tests (if there are any). |
I have experienced the same outcome on my Mac now, I'm pretty sure the difference is due to the failed tests. Regarding |
The way gtest-parallel works is that as long as tests are passing, the output to stdout is of the following format:
if a test fails, the gtest log of that test will be displayed and then gtest-parallel will continue executing and print a line of the above format again, see this gist for an example output. |
If "make watch-log" does not work, the rule should be deleted. Personally, I use "watch-log" to see which (if any tests) have failed so far. The failed tests are listed at the top (I think) and I see which tests have failed. This allows me to abort the test earlier if I see tests that are failing which I do not expect. I do not know if there is anything comparable with gtest-parallel (or ctest for that matter). |
751aba3
to
0ee37ec
Compare
Deleted the rule. gtest-parallel prints the failed tests in the same window you are running |
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.
LGTM
@Yuval-Ariel Can we merge this? |
…122) Instead, use gtest-parallel as a bundled script for running both gtest tests and non-gtest tests (such as c_test, for example).
0ee37ec
to
386355d
Compare
how has this been tested? |
@Yuval-Ariel Cannot speak for general testing but I ran this on my Mac and it works better than the original. Performance does not change but the old code had problems with test files that contained no tests. |
@AmnonHanuhov ? i want to make sure this does not break anything for existing users using different platforms |
This was tested on both Linux and mac, by Udi, myself, and Mark, in both platforms I had no problems running the tests in parallel with gtest-parallel and even saw small improvements in terms of the time it took to run the tests, compared to GNU parallel. |
Instead, use gtest-parallel as a bundled script for running both gtest tests and non-gtest tests (such as c_test, for example).