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

Run tests in current binary directory, not global source directory #206

Merged
merged 1 commit into from
Nov 22, 2019
Merged

Run tests in current binary directory, not global source directory #206

merged 1 commit into from
Nov 22, 2019

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Oct 22, 2019 •
edited
Loading

For consistency with CMake add_test, run tests in the project binary directory, not the top-level CMakeLists.txt

@rotu rotu changed the title Run tests in project source directory, not global source directory WIP: Run tests in project source directory, not global source directory Oct 23, 2019
@dirk-thomas
Copy link
Contributor

CI builds:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Please also sign-off the commits to pass DCO.

@rotu
Copy link
Contributor Author

rotu commented Oct 23, 2019

Those CI failures aren't as bad as I figured. Want me to go ahead with CMAKE_CURRENT_BINARY_DIR or stick with the "safer" source dir?

@dirk-thomas
Copy link
Contributor

Want me to go ahead with CMAKE_CURRENT_BINARY_DIR

Yes, because that is what matches CMake standard behavior.

@rotu
Run tests in current binary directory, not global source directory
561eb58 
Switch to CMAKE_CURRENT_BINARY_DIR for consistency with CTest

Signed-off-by: Dan Rose <dan@digilabs.io>
@rotu rotu changed the title WIP: Run tests in project source directory, not global source directory WIP: Run tests in current binary directory, not global source directory Oct 23, 2019
rotu added a commit to RoverRobotics-forks/launch that referenced this pull request Oct 23, 2019
@rotu
Use ament_cmake_copyright
e1c2374 
This removed the dependency on pytest and prevents an anticipated breakage from ament/ament_cmake#206
Signed-off-by: Dan Rose <dan@digilabs.io>
rotu added a commit to RoverRobotics-forks/launch that referenced this pull request Oct 23, 2019
@rotu
Use ament_cmake_copyright
a9d07f4 
More inline with how other packages do it and avert an anticipated breakage from ament/ament_cmake#206
Signed-off-by: Dan Rose <dan@digilabs.io>
@rotu rotu mentioned this pull request Oct 23, 2019
Merged
rotu added a commit to RoverRobotics-forks/rcutils that referenced this pull request Oct 23, 2019
@rotu
Specify working directory for filesystem test
338d395 
 Prevent anticipated breakage from ament/ament_cmake#206
Signed-off-by: Dan Rose <dan@digilabs.io>
@rotu rotu mentioned this pull request Oct 23, 2019
Merged
rotu added a commit to RoverRobotics-forks/rcl that referenced this pull request Oct 23, 2019
@rotu
Specify test working directory
97c35d1 
Avert breakage from ament/ament_cmake#206
Signed-off-by: Dan Rose <dan@digilabs.io>
@rotu rotu mentioned this pull request Oct 23, 2019
Merged
@dirk-thomas
Copy link
Contributor

Want me to go ahead with CMAKE_CURRENT_BINARY_DIR

Yes, because that is what matches CMake standard behavior.

I am sorry but I misread the value. I thought the goal was to change it to CMAKE_CURRENT_SOURCE_DIR. I would be ok with that since it only affect the use case where the package is built in a sub project.

I don't think changing the value to CMAKE_CURRENT_BINARY_DIR now is feasible since we have passed the API freeze for the upcoming release and this is certainly a breaking change.

@rotu
Copy link
Contributor Author

rotu commented Oct 23, 2019 •
edited
Loading

...That's why I asked 👎

This PR can hold off until F. The supporting PRs can be safely merged now anyway.

@dirk-thomas
Copy link
Contributor

This PR can hold off until F.

How about creating a separate PR to change it to CMAKE_CURRENT_SOURCE_DIR which could be merged right away? Independent of this PR later being accepted for F.

@rotu
Copy link
Contributor Author

rotu commented Oct 23, 2019

How about creating a separate PR to change it to CMAKE_CURRENT_SOURCE_DIR which could be merged right away? Independent of this PR later being accepted for F.

Now that I've done the supporting work for CMAKE_CURRENT_BINARY_DIR, I don't see the value in an intermediate PR. If you find it useful, go ahead.

@dirk-thomas
Copy link
Contributor

I don't see the value in an intermediate PR.

Wouldn't that be necessary to build Eloquent using your sub project approach?

@rotu
Copy link
Contributor Author

rotu commented Oct 23, 2019

I'm happy to use this branch as-is with my Eloquent tree, even if it's not slated for Eloquent release

@rotu rotu changed the title WIP: Run tests in current binary directory, not global source directory Run tests in current binary directory, not global source directory Oct 23, 2019
hidmic pushed a commit to ros2/launch that referenced this pull request Oct 23, 2019
@rotu @hidmic
Use ament_cmake_copyright (#349)
7ea6cc3 
More inline with how other packages do it and avert an anticipated breakage from ament/ament_cmake#206
Signed-off-by: Dan Rose <dan@digilabs.io>
dirk-thomas pushed a commit to ros2/rcl that referenced this pull request Oct 23, 2019
@rotu @dirk-thomas
Specify test working directory (#529)
c173935 
Avert breakage from ament/ament_cmake#206
Signed-off-by: Dan Rose <dan@digilabs.io>
dirk-thomas pushed a commit to ros2/rcutils that referenced this pull request Oct 23, 2019
@rotu @dirk-thomas
Specify working directory for filesystem test (#185)
c688c4a 
Prevent anticipated breakage from ament/ament_cmake#206
Signed-off-by: Dan Rose <dan@digilabs.io>
@dirk-thomas
Copy link
Contributor

With Eloquent being branched master is targeting F-turtle so this can be merged now. Thanks for the patch.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status (unrelated flaky test)

@dirk-thomas dirk-thomas merged commit 83e1b81 into ament:master Nov 22, 2019
@dirk-thomas dirk-thomas mentioned this pull request Nov 25, 2019
Merged
@rotu rotu deleted the patch-2 branch November 30, 2019 21:48
nuclearsandwich pushed a commit that referenced this pull request Mar 20, 2020
@rotu @nuclearsandwich
Run tests in current binary directory, not global source directory (#206
331a756 
)

Switch to CMAKE_CURRENT_BINARY_DIR for consistency with CTest

Signed-off-by: Dan Rose <dan@digilabs.io>
@nuclearsandwich nuclearsandwich mentioned this pull request Mar 23, 2020
Merged
nuclearsandwich pushed a commit that referenced this pull request Mar 23, 2020
@rotu @nuclearsandwich
Run tests in current binary directory, not global source directory (#206
4354d62 
)

Switch to CMAKE_CURRENT_BINARY_DIR for consistency with CTest

Signed-off-by: Dan Rose <dan@digilabs.io>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>
nuclearsandwich added a commit that referenced this pull request Apr 9, 2020
@nuclearsandwich
Revert "Run tests in current binary directory, not global source dire…
a7ffd1d 
…ctory (#206)"

This reverts commit 4354d62.
nuclearsandwich added a commit that referenced this pull request Apr 11, 2020
@nuclearsandwich
Revert "Run tests in current binary directory, not global source dire…
3ce9599 
…ctory (#206)"

This reverts commit 4354d62.

Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>
nuclearsandwich added a commit that referenced this pull request Apr 11, 2020
@nuclearsandwich @pbaughman
@dirk-thomas @rotu @j-rivero
[Dashing] Backport junit xml fixes for updated xunit plugin on c… (#230)
8c4ddc6 
* Add runner option to ament_add_test (#174)

* ament_cmake allow speficiation of a different test runner

  - By default, still uses run_test.py
  - Example use case: ament_cmake_ros can use a test runner that
    sets a ROS_DOMAIN_ID

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

* ament_cmake move run_test.py to a python module

  - This should let us see the history

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>

* ament_cmake refactor run_test.py into an importable python module

  - Adds an ament_cmake_test python package

Signed-off-by: Pete Baughman <pete.baughman@apex.ai>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* improve handling of encoding (#181)

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* use deterministic order for updated env vars (#196)

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* Run tests in current binary directory, not global source directory (#206)

Switch to CMAKE_CURRENT_BINARY_DIR for consistency with CTest

Signed-off-by: Dan Rose <dan@digilabs.io>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* remove status attribute from result XML, add skipped tag instead (#218)

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* Declare AMENT_TEST_RESULTS_DIR as a PATH (#221)

Signed-off-by: Dan Rose <dan@digilabs.io>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* Generate xunit files valid for the junit10.xsd

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

* Revert "Run tests in current binary directory, not global source directory (#206)"

This reverts commit 4354d62.

Signed-off-by: Steven! Ragnarök <steven@nuclearsandwich.com>

Co-authored-by: Peter Baughman <dblue135@yahoo.com>
Co-authored-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>
Co-authored-by: Dan Rose <rotu@users.noreply.github.com>
Co-authored-by: Jose Luis Rivero <jrivero@osrfoundation.org>
@icolwell-as icolwell-as mentioned this pull request Oct 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirk-thomas dirk-thomas dirk-thomas left review comments

Assignees

@dirk-thomas dirk-thomas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rotu @dirk-thomas