Skip to content

Migrate launch tests to new launch_testing features & API#340

Merged
hidmic merged 11 commits intomasterfrom
hidmic/import_launch_testing
Apr 30, 2019
Merged

Migrate launch tests to new launch_testing features & API#340
hidmic merged 11 commits intomasterfrom
hidmic/import_launch_testing

Conversation

@hidmic
Copy link
Copy Markdown
Contributor

@hidmic hidmic commented Mar 25, 2019

Connected to ros2/launch#208.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic added the in progress Actively being worked on (Kanban column) label Mar 25, 2019
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic force-pushed the hidmic/import_launch_testing branch from 56e9ef9 to f23ba1d Compare March 27, 2019 20:14
@pbaughman
Copy link
Copy Markdown
Contributor

It might be worth declaring the tests with proc_info/proc_output as an argument. That works as well as referencing the magic local field self.proc_info/self.proc_output

like:

def test_@TEST_NUM_NODES@_nodes(self, proc_info):
    launch_testing.asserts.assertExitCodes(proc_info)

I added proc_info and proc_output to the test class before your suggestion to use arguments for the context returned from the test description (I like how you're using locals() in the test description, btw). It's something I was considering deprecating because passing as arguments feels so much cleaner.

hidmic added 4 commits April 12, 2019 15:18
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Apr 12, 2019
hidmic added 3 commits April 15, 2019 11:48
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@jacobperron jacobperron self-requested a review April 25, 2019 21:35
Copy link
Copy Markdown
Member

@jacobperron jacobperron 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. Just a couple nitpicks.

Copy link
Copy Markdown
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

}
finally:
node.destroy_node()
TEST_CASES = {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably out of the scope of this PR, but IMHO this migrated test looks a little difficult to read. I mean, I needed to read the comments in launch/launch_testing/example_test_context.test.py to understand what was happening here (maybe, there is better documentation somewhere).
Passing test argument in the returned test context it's not quite intuitive. Also, it would be nice to have better parameterization support.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I know it's hard to read, I did my best to not split it up while keeping some of the unittest like structure. Alternatively, we could partially recreate what pytest does with decorators to inject the test functions like it used to be but it seemed a bit too much for a migration.

hidmic added 2 commits April 26, 2019 13:49
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic merged commit f15cc2a into master Apr 30, 2019
@delete-merged-branch delete-merged-branch bot deleted the hidmic/import_launch_testing branch April 30, 2019 22:47
@hidmic hidmic removed the in review Waiting for review (Kanban column) label Apr 30, 2019
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