Skip to content
This repository was archived by the owner on May 31, 2025. It is now read-only.

Add test migration rule#1010

Merged
dirk-thomas merged 1 commit intoros:lunar-develfrom
clearpathrobotics:add_test_migration_rule
Mar 20, 2017
Merged

Add test migration rule#1010
dirk-thomas merged 1 commit intoros:lunar-develfrom
clearpathrobotics:add_test_migration_rule

Conversation

@efernandez
Copy link
Copy Markdown
Contributor

@efernandez efernandez commented Mar 2, 2017 •
edited
Loading

This adds a test for #996, which I'm going to close a create a new one rebased on top of this to show it actually fixes the issue (see #1011). I'd also target lunar-devel.

As a reminder, this explains the corner case this test covers now.

Description of the buggy corner case not handled on the rosbag migration tool

Consider the following scenario for a message G with the following versions:

G0 -- rule.bmr --> G1 -- auto-generated rule --> G3

where G3 is the current msg, and G2 is the bag message that needs migration.

We need to check for a migration rule like this first:

G2 --> G3

instead of the backwards ones that were generated before this bug fix:

G2 --> G1

Note that the lazy check to mark the rules as valid in the make_update_rule (see https://github.com/clearpathrobotics/ros_comm/blob/kinetic-devel-cpr/tools/rosbag/src/rosbag/migration.py#L1123) method makes that rule pass initially, but later it fails because there's no rule for the sub fields.

@mikepurvis @dirk-thomas

Note this PR is on top of #1009

@efernandez
Copy link
Copy Markdown
Contributor Author

efernandez commented Mar 2, 2017 •
edited
Loading

Please note that this PR should fail.

Here I just want to:

  1. Show the test on its own
  2. Show that this test fails with the current code

I'm going to create another PR on top of this one that should pass.
The intention is to close this one and merge the other one.

I'd prefer that you review the test changes here, rather than on the PR I'm going to create with the fix.

Also remember that #1009 should be reviewed and merged before this one.

Indeed, these two tests should fail:

======================================================================
FAIL: test_converged_gen4 (migrate_test.MigrationTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test/migrate_test.py", line 324, in test_converged_gen4
    self.do_converged(4)
  File "/home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test/migrate_test.py", line 294, in do_converged
    self.assertTrue(not False in [m[1] == [] for m in res], 'Bag not ready to be migrated')
AssertionError: Bag not ready to be migrated

======================================================================
FAIL: test_migrated_mixed_gen4 (migrate_test.MigrationTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test/migrate_test.py", line 243, in test_migrated_mixed_gen4
    self.do_migrated_mixed(4)
  File "/home/eperdomo/dev/ws/clearpath_ws/build/test_rosbag/test/migrate_test.py", line 215, in do_migrated_mixed
    self.assertTrue(not False in [m[1] == [] for m in res], 'Bag not ready to be migrated')
AssertionError: Bag not ready to be migrated

----------------------------------------------------------------------

And now that the PR test jenkins job finished you can see it's exactly failing on those two, as expected: http://build.ros.org/job/Lpr__ros_comm__ubuntu_xenial_amd64/25/testReport/

And http://build.ros.org/job/Lpr__ros_comm__ubuntu_xenial_amd64/26/ shows that #1011 fixes it.

This was referenced Mar 2, 2017
Merged
Closed
efernandez
efernandez commented Mar 2, 2017
View reviewed changes
test/test_rosbag/bag_migration_tests/CMakeLists.txt Outdated
catkin_download_test_data(download_data_test_converged_gen1.bag http://download.ros.org/data/test_rosbag/converged_gen1.bag FILENAME test/converged_gen1.bag MD5 8e3524157d31b5761ac951fe16e03e12 )
catkin_download_test_data(download_data_test_converged_gen2.bag http://download.ros.org/data/test_rosbag/converged_gen2.bag FILENAME test/converged_gen2.bag MD5 0ad4041d2e3bab8262c12020ec3e048e )
catkin_download_test_data(download_data_test_converged_gen3.bag http://download.ros.org/data/test_rosbag/converged_gen3.bag FILENAME test/converged_gen3.bag MD5 90dd16cea5c51fca65be617654fb6b76 )
#catkin_download_test_data(download_data_test_converged_gen4.bag http://download.ros.org/data/test_rosbag/converged_gen4.bag FILENAME test/converged_gen4.bag MD5 c88ac1f4a9a5eebd667ec55224988e40 )
Copy link
Copy Markdown
Contributor Author

@efernandez efernandez Mar 2, 2017

Choose a reason for hiding this comment

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

@dirk-thomas These 6 bag files are now in Dropbox. Could you please move them to download.ros.org, please? Then I'd update the PR.

Copy link
Copy Markdown
Contributor Author

@efernandez efernandez Mar 6, 2017

Choose a reason for hiding this comment

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

ping @dirk-thomas

Copy link
Copy Markdown
Member

@dirk-thomas dirk-thomas Mar 7, 2017

Choose a reason for hiding this comment

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

Sorry, for the delay. The 6 files are available via http://download.ros.org/data/test_rosbag/ now.

Copy link
Copy Markdown
Contributor Author

@efernandez efernandez Mar 7, 2017

Choose a reason for hiding this comment

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

Thanks. PR updated to use them.

Also rebased/updated #1011 that should pass the test; remember that this PR (#1010 it's only here to show that the current code fails to handle the new test provided)

Copy link
Copy Markdown
Member

@dirk-thomas dirk-thomas Mar 8, 2017

Choose a reason for hiding this comment

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

👍

@efernandez efernandez force-pushed the add_test_migration_rule branch 2 times, most recently from 469ac1c to 255f047 Compare March 6, 2017 16:39
@efernandez efernandez mentioned this pull request Mar 6, 2017
Merged
@efernandez efernandez force-pushed the add_test_migration_rule branch 2 times, most recently from 7e98073 to 66818e7 Compare March 9, 2017 00:06
Add message generation 4 to test_rosbag
8cbbe6b 
This allows to check for the following case:

Consider the following scenario for a message G with the following
versions:

G0 -- rule.bmr --> G1 -- auto-generated rule --> G3
where G3 is the current msg, and G2 is the bag message that needs
migration.

We need to check for a migration rule like this first:

G2 --> G3
instead of the backwards ones that were generated before this bug fix:

G2 --> G1

Note that the lazy check to mark the rules as valid in the
make_update_rule (see
https://github.com/clearpathrobotics/ros_comm/blob/kinetic-devel-cpr/tools/rosbag/src/rosbag/migration.py#L1123)
method makes that rule pass initially, but later it fails because
there's no rule for the sub fields.

Here, in test_rosbag, the messages that corresponds to G in the example
above, is MessageMixed.
@efernandez efernandez force-pushed the add_test_migration_rule branch from 66818e7 to 8cbbe6b Compare March 15, 2017 21:21
@dirk-thomas dirk-thomas merged commit 8cbbe6b into ros:lunar-devel Mar 20, 2017
@efernandez efernandez deleted the add_test_migration_rule branch March 20, 2017 20:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@efernandez @dirk-thomas