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

Disable long-running buildsystem tests by default #97

Merged
merged 4 commits into from
Jun 4, 2020

Conversation

mjcarroll
Copy link
Contributor

From #92, this disables several tests that take a while to run and aren't correctly detected by CMake's change-detection. This adds a new option BUILDSYSTEM_TESTING, which will re-enable these tests when true, and enables it in pipelines.

Tested locally with:

$ cmake .. -DBUILDSYSTEM_TESTING=1
$ time make
make  81.50s user 23.50s system 99% cpu 1:45.22 total
$ cmake ..
$ time make
make  0.18s user 0.07s system 99% cpu 0.254 total

Depends on gazebo-tooling/action-gz-ci#9

@mjcarroll mjcarroll requested a review from chapulina May 19, 2020 13:34
@mjcarroll mjcarroll self-assigned this May 19, 2020
@mjcarroll mjcarroll requested a review from mxgrey May 19, 2020 13:34
@mjcarroll
Copy link
Contributor Author

Note to reviewers, the action PR needs to land first, and then I will correct the branches in the workflow file and reduce it to single build with the full build invocation.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM if CI passes with the action's master branch.

Also we need to look into what happened to the Jenkins builds.

Finally we should consider enabling these tests on Jenkins.

.github/workflows/ci-bionic.yml Outdated Show resolved Hide resolved
@mjcarroll
Copy link
Contributor Author

Also we need to look into what happened to the Jenkins builds.

This looks like it built on a xenial machine. That doesn't seem correct, given that the minimum cmake version is way past that now.

@mjcarroll mjcarroll force-pushed the disable_long_tests branch 2 times, most recently from 9d851e6 to 175be6d Compare May 19, 2020 16:38
@mjcarroll mjcarroll marked this pull request as ready for review May 19, 2020 16:38
@chapulina
Copy link
Contributor

This looks like it built on a xenial machine.

Oh I just saw that this PR builds on top of #96 . See my comment on that PR for why we can't remove the pipelines file just yet.

@mjcarroll mjcarroll changed the base branch from remove_pipelines to master May 19, 2020 16:47
mjcarroll and others added 3 commits May 19, 2020 11:47
Signed-off-by: Michael Carroll <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll
Copy link
Contributor Author

Oh I just saw that this PR builds on top of #96 . See my comment on that PR for why we can't remove the pipelines file just yet.

Rebased and force-pushed.

@mjcarroll
Copy link
Contributor Author

Unless we get the flag turned on in jenkins, I would expect the other CI jobs to fail because there is no test output.

@mjcarroll
Copy link
Contributor Author

Unless we get the flag turned on in jenkins, I would expect the other CI jobs to fail because there is no test output.

Ah, this will only impact Windows, because we have a test available in UNIX platforms.

@scpeters
Copy link
Member

for Ubuntu, I think we could set the BUILDING_EXTRA_CMAKE_PARAMS variable in https://github.com/ignition-tooling/release-tools/blob/master/jenkins-scripts/docker/ign_cmake-compilation.bash

@scpeters
Copy link
Member

@mjcarroll
Copy link
Contributor Author

Since this passes everything but Windows, I'm going to merge. Windows CI will go green when this lands: gazebo-tooling/release-tools#210

@scpeters
Copy link
Member

scpeters commented Jun 1, 2020

Since this passes everything but Windows, I'm going to merge

I'd prefer to wait for the release-tools PR to land before merging this

@mjcarroll
Copy link
Contributor Author

I'd prefer to wait for the release-tools PR to land before merging this

In retrospect, I can't merge it because of the branch protection, so you'll get your wish.

@scpeters
Copy link
Member

scpeters commented Jun 1, 2020

@scpeters
Copy link
Member

scpeters commented Jun 2, 2020

I've cherry-picked these commits onto ign-cmake2 in disable_long_tests_2:

Signed-off-by: Steve Peters <[email protected]>
@mjcarroll mjcarroll merged commit af0dd75 into master Jun 4, 2020
@mjcarroll mjcarroll deleted the disable_long_tests branch June 4, 2020 15:39
@scpeters
Copy link
Member

scpeters commented Jun 4, 2020

Can you backport to ign-cmake2?

mjcarroll added a commit that referenced this pull request Jun 4, 2020
Co-authored-by: Louise Poubel <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
chapulina added a commit that referenced this pull request Jun 5, 2020
Co-authored-by: Louise Poubel <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
scpeters added a commit that referenced this pull request Aug 24, 2021
Co-authored-by: Louise Poubel <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
chapulina added a commit that referenced this pull request Aug 24, 2021
* Part of gazebo-tooling/release-tools#203
* Disable long-running buildsystem tests by default (#97) (#100)
* backport triage and labeler

Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
Co-authored-by: Michael Carroll <[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.

4 participants