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.

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

Refactor GfxReplayTest::testSimulatorIntegration #1855

Merged
merged 3 commits into from
Sep 8, 2022

Conversation

0mdc
Copy link
Contributor

@0mdc 0mdc commented Sep 6, 2022

Motivation and Context

The GfxReplayTest::testSimulatorIntegration test is currently using the same simulator instance to record and play a playback file. This doesn't model real use cases and may hide issues.

This changeset refactors the test so that separate simulator instances record and play.

How Has This Been Tested

Tests are successful locally.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@0mdc 0mdc self-assigned this Sep 6, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 6, 2022
…ulator instance record and play the playback file.
Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

Refactor looks good to me, but curious about the log output here for CI testing.

@mosra I see the following in the CI log, but number 5 should be testSimulatorIntegration and instead displays as getNumberOfChildrenOfRoot which is a helper function called within the latter test. Is this a bug or am I missing something?

Test command: /home/circleci/project/habitat-sim/build/tests/GfxReplayTest
9: Environment variables: 
9:  HABITAT_SIM_LOG=quiet
9:  MAGNUM_LOG=QUIET
9: Test timeout computed to be: 10000000
9: Starting GfxReplayTest with 5 test cases...
9:     OK [1] testRecorder()
9:     OK [2] testPlayer()
9: [15:38:08:271680]:[Gfx] Player.cpp(37)::readKeyframesFromFile : File file_that_does_not_exist.json not found.
9:     OK [3] testPlayerReadMissingFile()
9: [15:38:08:271933]:[IO] Json.cpp(70)::parseJsonFile : Parse error reading /home/circleci/project/habitat-sim/src/../data/./gfx_replay_test.json Error code 4 at 1
9: [15:38:08:272044]:[Gfx] Player.cpp(44)::readKeyframesFromFile : Failed to parse keyframes from /home/circleci/project/habitat-sim/src/../data/./gfx_replay_test.json .
9:     OK [4] testPlayerReadInvalidFile()
9: [15:38:08:276886]:[Core] ManagedContainer.h(143)::registerObject : <Scene Instance> : No valid handle specified to register this Scene Instance managed object. Aborting.
9: [15:38:08:276960]:[Core] ManagedContainer.h(143)::registerObject : <Scene Instance> : No valid handle specified to register this Scene Instance managed object. Aborting.
9:     OK [5] getNumberOfChildrenOfRoot()
9: Finished GfxReplayTest with 0 errors out of 79 checks.
 9/24 Test  #9: GfxReplayTest ....................   Passed    0.32 sec

@mosra
Copy link
Collaborator

mosra commented Sep 7, 2022

Not a bug, but a consequence of how the test function name is captured via the CORRADE_VERIFY() / CORRADE_COMPARE() macros -- the first time the macro gets used in a test case, it captures the name of the surrounding function. You have two options:

  1. Remove the CORRADE_VERIFY(lastRootChild); from getNumberOfChildrenOfRoot(), or change it to e.g. CORRADE_INTERNAL_ASSERT(lastRootChild). That sounds like the more robust solution to me -- the helper doesn't really verify anything, just returns a value (that gets later verified), so it shouldn't need to use the verification macros in my opinion.

  2. Make sure some macro gets called from within testSimulatorIntegration() before getNumberOfChildrenOfRoot() is called, or just add something like

    CORRADE_VERIFY(true); /* capture correct function name */

    at the very top of the testSimulatorIntegration() test case. This is rather prone to breaking again in the future, tho, especially when getNumberOfChildrenOfRoot() gets used in more test cases.

Copy link
Contributor

@aclegg3 aclegg3 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, let's merge it.

@0mdc 0mdc merged commit 8bc2fcc into facebookresearch:main Sep 8, 2022
@0mdc 0mdc deleted the replay-test-refactor branch September 8, 2022 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants