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

add support for {#} similar to parallel #89

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

1480c1
Copy link

@1480c1 1480c1 commented Jun 17, 2022

allows for --gtest_output=xml:reports/report_{#}.xml to be used instead
of relying on googletest itself to lock a unique filename and using that

usage would be something like

python \
    gtest_parallel.py \
    --print_test_times \
    ./unit_tests \
    -- \
    --gtest_output="xml:reports/{#}.xml"

and then uploading the reports/*.xml files for junit reports or using something like junitparser to merge them together.

Formatted using yapf, so that's where the extra spaces come from.

@1480c1
Copy link
Author

1480c1 commented Jun 17, 2022

This mainly targets a different direction than #81 as rather than attempting to merge the resulting output inside the script, it just allows the script to have the xmls be produced to different files, but does allow the script to be used inside gitlab-ci by using something like

  artifacts:
    when: always
    reports:
      junit: reports/*.xml

in their yml file

@1480c1 1480c1 marked this pull request as ready for review June 17, 2022 22:38
@pbos
Copy link
Collaborator

pbos commented Jun 22, 2022

Intuitively I think #81 would make more sense as an approach to support, but it stalled out. Would this solve something else than #81 does? At first glance it looks like a workaround for #81 having stalled.

This part to me was somewhat worrying and I haven't seen anything since that comment:

"The merging of the xml fails, if one of the tests crashes the test itself. Which happens for me with glog and CHECK(...). This creates a SIGABRT. The merge fails because no xml is created for that test."

@1480c1
Copy link
Author

1480c1 commented Jun 22, 2022

Would this solve something else than #81 does? At first glance it looks like a workaround for #81 having stalled.

Well, the original intention for this PR was something of a workaround for #81 being stalled, but in a sense, is more general since it does not care about if the {#} is in --gtest_output= or not and can be used elsewhere in the command line and can be used in --gtest_filter= if for some reason you have a group of tests that uses numbers in its name.

This was originally inspired by parallel since at one point I fiddled around with filtering the output of --gtest_list_tests with awk to get the full test names and then piping that into parallel to then call the unit test binaries directly with --gtest_output="xml:reports/{#}.xml" to replicate gtest_parallel.py, but I found out that due to the sheer number of small tests we have, it often took longer for the init than it actually took for the test.

The primary reason why I did not look into continuing #81 is simply because of my lack of python skill, so I have no confidence in being able to complete what that PR attempts to accomplish in the same time period as what it took for this modification

@pbos
Copy link
Collaborator

pbos commented Jun 24, 2022

I think the real solution should end up solving #81. I think this is better suited on a local branch, I'm wary of adding parameter support in general for things that are not supported in upstream gtest (and am very aware that I've done so in the past, imo -r shouldn't exist, but rather be just --gtest_repeat).

allows for `--gtest_output=xml:reports/report_{#}.xml` to be used instead
of relying on googletest itself to lock a unique filename and using that

Signed-off-by: Christopher Degawa <[email protected]>
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.

2 participants