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.

Sign up for GitHub

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

Jump to bottom

Disable rendering tests on Mac when building debug. #257

Merged
merged 4 commits into from
Apr 27, 2018
Merged

Disable rendering tests on Mac when building debug. #257

merged 4 commits into from
Apr 27, 2018

Conversation

nuclearsandwich
Copy link
Member

Display tests currently fail due to #246.
In order to make it easier to triage CI failures, disable these tests in that configuration pending resolution of the failure.

Temporarily resolves #245.

@nuclearsandwich
Disable rendering tests on Mac when building debug.
72ea96e 
Display tests currently fail due to #246.
In order to make it easier to triage CI failures, disable these tests
in that configuration pending resolution of the failure.
@nuclearsandwich nuclearsandwich added the in progress Actively being worked on (Kanban column) label Apr 25, 2018
@nuclearsandwich nuclearsandwich self-assigned this Apr 25, 2018
@wjwwood wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 25, 2018
wjwwood
wjwwood approved these changes Apr 25, 2018
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm, should be temporary until we can resolve the debug loading. Thanks.

@wjwwood
Copy link
Member

wjwwood commented Apr 25, 2018

@nuclearsandwich have your run CI to show that these are disabled correctly (And still work when not in Debug)?

mikaelarguedas
mikaelarguedas reviewed Apr 25, 2018
View reviewed changes
rviz_rendering_tests/CMakeLists.txt Outdated
PUBLIC src/rviz_rendering_tests)
target_link_libraries(test_rviz_rendering_tests
ament_index_cpp::ament_index_cpp)
if (APPLE AND NOT CMAKE_BUILD_TYPE STREQUAL "Debug")
Copy link
Member

Choose a reason for hiding this comment

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

Are the tests now disabled for Linux ? (aka if NOT APPLE but DISPLAYPRESENT)

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah... Should probably be NOT APPLE OR NOT CMAKE_BUILD_TYPE STREQUAL "Debug"?

Copy link
Member

Choose a reason for hiding this comment

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

yep or NOT (APPLE AND CMAKE_BUILD_TYPE STREQUAL "Debug"). I find the latter easier to read but it's very subjective

Copy link
Member

Choose a reason for hiding this comment

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

Yours sounds better.

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch here. If we opt to keep the nested conditional approach I'll change it to @mikaelarguedas's condition.

nuclearsandwich
nuclearsandwich commented Apr 25, 2018
View reviewed changes
rviz_rendering_tests/CMakeLists.txt Outdated
@@ -70,7 +70,7 @@ if(BUILD_TESTING)
rviz_ogre_vendor::OgreMain
rviz_rendering::rviz_rendering
resource_retriever::resource_retriever
)
)
Copy link
Member Author

Choose a reason for hiding this comment

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

I can smudge out this whitespace change before merging.

@nuclearsandwich
Copy link
Member Author

Checking that tests are not run in Debug: Build Status

mikaelarguedas
mikaelarguedas reviewed Apr 25, 2018
View reviewed changes
rviz_rendering_tests/CMakeLists.txt
@@ -59,7 +59,7 @@ if(BUILD_TESTING)
set(DISPLAYPRESENT FALSE)
endif()

if(APPLE OR DISPLAYPRESENT OR EnableDisplayTests STREQUAL "True")
if((APPLE AND NOT CMAKE_BUILD_TYPE STREQUAL "Debug") OR DISPLAYPRESENT OR EnableDisplayTests STREQUAL "True")
Copy link
Member

Choose a reason for hiding this comment

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

That is now a very long line.

I think that's better to add a temporary extra conditional with a TODO as we expect it to disappear as soon as the tests are fixed in Debug. I assume that on the other hand the original APPLE OR DISPLAYPRESENT OR EnableDisplayTests STREQUAL "True" will stay as is for the foreseeable future, is that correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have strong feelings either way. The extra conditional optimized for preserving existing logic, this optimizes for minimal diff churn.

@nuclearsandwich
Copy link
Member Author

Checking that tests are not run in Debug: Build Status

The tests still ran and failed with the same error. I'm not entirely sure why. Does CMAKE_BUILD_TYPE need to be evaluated, i.e. "${CMAKE_BUILD_TYPE}" STREQUAL "Debug" and if so do we need to evaluate EnableDisplayTests as well in order to make that part of the condition work?

@mikaelarguedas
Copy link
Member

Does CMAKE_BUILD_TYPE need to be evaluated,

I don't think so, at least we use it not evaluated in other places and it seems to be doing the right thing. Maybe another part of the condition evaluates to TRUE ?
I would try to either:

  • run cmake on (DISPLAYPRESENT OR EnableDisplayTests STREQUAL "True") only and see if it evalutes to True on Mac
  • split the conditional in 2 and put (APPLE AND NOT CMAKE_BUILD_TYPE STREQUAL "Debug") inside the other condition to see which condition we enter
  • print the other variables used in this condition to see which one is true

@wjwwood
Copy link
Member

wjwwood commented Apr 25, 2018

It certainly wouldn't be wrong to do "${CMAKE_BUILD_TYPE}" STREQUAL "Debug", but I agree the other should work too...

@nuclearsandwich
Copy link
Member Author

This construct is actually part of rviz_common, rviz_default_plugins, rviz_rendering, and rviz_rendering_tests but I'd only made the change in rviz_rendering_tests.
232268f makes the change for the other packages.

🍎 (Debug) Build Status
🍎 (None) Build Status
🎐 Build Status
🐧 Build Status

@mikaelarguedas
Copy link
Member

It appears that the lint_cmake failure of the previous round of CI has not been addressed in the last commit

@wjwwood
Copy link
Member

wjwwood commented Apr 27, 2018

@nuclearsandwich since the only failure was the linter failure and it was just whitespace change, I'm ok if you want to merge this now without new CI.

@nuclearsandwich
Copy link
Member Author

With @mikaelarguedas's assistance I verified the fix for the linter error locally and a build (below) is running to verify it in CI.

For the builds above, Display tests ran on Mac with build type none but did not run on the Debug build or Windows or Linux builds. So this change should be good once the final CI check completes. Hopefully it won't stick around too long.

Build Status

@nuclearsandwich nuclearsandwich merged commit e8181a4 into ros2 Apr 27, 2018
@nuclearsandwich nuclearsandwich removed the in review Waiting for review (Kanban column) label Apr 27, 2018
@mikaelarguedas mikaelarguedas deleted the disable-display-tests-for-mac-debug branch April 27, 2018 19:05
botteroa-si added a commit to bosch-io/rviz that referenced this pull request May 8, 2018
@botteroa-si
Revert "Disable rendering tests on Mac when building debug. (ros2#257)"
d6e4ac3 
This reverts commit e8181a4.
wjwwood pushed a commit that referenced this pull request May 16, 2018
@botteroa-si @wjwwood
Fix debug build on Mac (#258)
d49b17d 
* Revert "Disable rendering tests on Mac when building debug. (#257)"

This reverts commit e8181a4.

* Fix debug build on macOS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood approved these changes

@mikaelarguedas mikaelarguedas mikaelarguedas left review comments

Assignees

@nuclearsandwich nuclearsandwich

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Assert triggering in nightly MacOS debug builds
3 participants
@nuclearsandwich @wjwwood @mikaelarguedas