Skip to content

Conversation

mrpropellers
Copy link
Contributor

@mrpropellers mrpropellers commented May 21, 2021

Proposed change(s)

This PR includes two major additions:
First, a new MonoBehaviour called TargetPlacement was added and attached to the TargetPlacement prefab. The purpose of this script is to track the state of TargetPlacement with respect to the expected Target to be placed within its bounds. This state tracking allows for automated and visual confirmation that the Pick and Place task completed successfully (the Target has been placed at rest in the TargetPlacement zone). Unit tests are also added for this class.

I've added scripts to the .yamato directory that modify the state of the PickAndPlaceProject to reproduce the configuration reached after finishing Part 3 of the tutorial and add the INTEGRATION_TEST scripting define to the ProjectSettings.asset file (this is the only difference between IntegrationTestSettings.asset and the original in the ProjectSettings directory).

Tested locally and on Yamato until same result was achieved.

Useful links (GitHub issues, JIRA tickets, forum threads, etc.)

Provide any relevant links here.

Types of change(s)

  • Added testing

Testing and Verification

It is a test, and it passes.

Test Configuration:

  • Unity 2020.2.0b9

  • Unity machine OS:
    image

  • ROS machine OS: Ubuntu 18.04 in Docker container from Pick and Place

  • ROS–Unity communication: loopback through Docker

Checklist

  • Ensured this PR is up-to-date with the dev branch
  • Created this PR to target the dev branch
  • Followed the style guidelines as described in the Contribution Guidelines
  • Added tests that prove my fix is effective or that my feature works

@mrpropellers mrpropellers force-pushed the devin/integration-test branch 7 times, most recently from 63217a6 to 54406ce Compare May 27, 2021 00:58
Comment on lines 83 to 89

#if INTEGRATION_TEST
int ReturnFive()
{
return 5;
}
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Experimental code - will remove.

@mrpropellers mrpropellers marked this pull request as ready for review May 27, 2021 18:31
@mrpropellers
Copy link
Contributor Author

Tests "passed," but on reviewing the output in the Yamato log it looks like the integration test was skipped. My guess is that the -editorTestCategories flag doesn't count as a testing request when the [Explicit] attribute evaluates whether a test should run. This will need a few more iterations...

@mrpropellers
Copy link
Contributor Author

Tests "passed," but on reviewing the output in the Yamato log it looks like the integration test was skipped. My guess is that the -editorTestCategories flag doesn't count as a testing request when the [Explicit] attribute evaluates whether a test should run. This will need a few more iterations...

https://unity-ci.cds.internal.unity3d.com/job/7007522

Line 4991 -- Test successfully executes!

@mrpropellers mrpropellers force-pushed the devin/integration-test branch from e0bceb5 to 77bd9bb Compare June 3, 2021 22:32
@mrpropellers mrpropellers force-pushed the devin/integration-test branch from 77bd9bb to d573f7f Compare June 3, 2021 22:34
- mkdir -p ./tutorials/pick_and_place/PickAndPlaceProject/Assets/Scripts && cp ./tutorials/pick_and_place/Scripts/*.cs ./tutorials/pick_and_place/PickAndPlaceProject/Assets
- utr/utr --testproject=./tutorials/pick_and_place/PickAndPlaceProject --editor-location=.Editor --reruncount=0 --artifacts_path=test-results --suite=playmode --suite=editor --platform=Editor --editorTestsCategories BuildTests
- git submodule update --init --recursive
# We must remove the Demo.cs script because the System.CodeDom assembly is not in the bokken .NET sdk
Copy link
Contributor

Choose a reason for hiding this comment

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

wow. i would not know this. Do you think we should ask bokken team to add System.CodeDom assembly ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have brought it up with them before and was basically told to figure it out myself. I think the issue is that utr automatically includes the -api-profile NET_Standard_2_0 argument in every test invocation, meaning that the API we're developing against is not the same as the API that test runner runs our tests against. I've raised this with the test framework team and hopefully we can figure out why that is.

@mrpropellers mrpropellers force-pushed the devin/integration-test branch from d1323fd to aba5df9 Compare June 8, 2021 18:34
@mrpropellers mrpropellers requested a review from sdiao June 8, 2021 18:50
@mrpropellers mrpropellers force-pushed the devin/integration-test branch from 02bd9ac to 44743e1 Compare June 10, 2021 23:02
Copy link
Contributor

@peifeng-unity peifeng-unity left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just have some questions to clarify.


[RequireComponent(typeof(MeshRenderer))]
[RequireComponent(typeof(BoxCollider))]
public class TargetPlacement : MonoBehaviour
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this class be used in the demo process or is it only for the integration test?

{
#if INTEGRATION_TEST
SetUpScene();
// TODO: This test could be made a PlayMode test once ImportRobot can use the PlayMode URDF import
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think the URDF-Importer 0.4.0 is ready to import the robot in the playmode?

Comment on lines +146 to +149
//m_RosConnection.overrideUnityIP = k_IpAddressLoopback;
//m_RosConnection.unityPort = k_UnityPort;
//m_RosConnection.awaitDataMaxRetries = k_NumAwaitDataRetries;
//m_RosConnection.awaitDataSleepSeconds = k_NumAwaitDataSleepSeconds;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to remove these lines?

@mrpropellers mrpropellers merged commit b4ce10e into dev Jun 14, 2021
peifeng-unity pushed a commit that referenced this pull request Jul 16, 2021
* AIRO-420 Adding integration tests to PickAndPlaceProject

* Updating version in tutorial and fixing TargetPlacement bug.
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.

3 participants