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

Set MAYA_APP_DIR for each test, pointing to unique folders. #1163

Merged
merged 3 commits into from
Feb 11, 2021

Conversation

seando-adsk
Copy link
Collaborator

No description provided.

@seando-adsk seando-adsk requested a review from pmolodo February 9, 2021 21:32
Comment on lines +320 to +329
# Set a temporary folder path for the MAYA_APP_DIR in which the
# maya profile will be created.
# Note: replace bad chars in test_name with _.
string(REGEX REPLACE "[:<>\|]" "_" SANITIZED_TEST_NAME ${test_name})
set(MAYA_APP_TEMP_DIR "${CMAKE_BINARY_DIR}/test/Temporary/${SANITIZED_TEST_NAME}")
# Note: ${WORKING_DIR} can point to the source folder, so don't use it
# in any env var that will write files (such as MAYA_APP_DIR).
set_property(TEST "${test_name}" APPEND PROPERTY ENVIRONMENT
"MAYA_APP_DIR=${MAYA_APP_TEMP_DIR}")
file(MAKE_DIRECTORY ${MAYA_APP_TEMP_DIR})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will put all the maya profiles into unique folders (by test name) in the folder ".../test/Temporary". This way you can easily delete them all if needed. I make sure to create the folders here as well. Actually the tail part of the folder will be created by Maya, but its easier to just make sure they are all created here.

Krystian pointed me to your attempt at this in the past and said you had some problems with test failures. I believe that could be because you used ${WORKING_DIR}, which sometimes points to the source folder (see line 98). Creating test files in the source folder would not be good. I'm not sure why WORKING_DIR would even point to the source folder - that seems incorrect.

I ran this multiple times on both my Windows and Linux machines and ran two of our internal builds on all platforms. I didn't see any test failures.

There are two other places where MAYA_APP_DIR is used directly. In those cases it will be set to a temporary folder created in the temp directory.

Copy link
Contributor

@pmolodo pmolodo Feb 9, 2021

Choose a reason for hiding this comment

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

The problem we saw earlier wasn't direct test failures, per-se... but just that the tests ran over a timeout when run in the preflight context. I never had any test failures when running locally, or really noticed much of a time difference when running the tests with the MAYA_APP_DIR set... but apparently it made a difference in whatever environment pre-flight was running.

Which isn't to say that your changes won't fix whatever the cause of that slowdown was - maybe it was something related to running out of the WORKING_DIR? But just be on the watch-out for it, I guess!

Anyway, I'll assign the bot user to get the tests to run, so hopefully we'll know soon!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay good to know. What we've seen is random test errors related to not being able to write to the MAYA_APP_DIR (because by default it will be set to the user folder). This change makes sure that its always in a writeable location (the build folder).

This change by itself won't have any impact on the test performance. In our internal build systems we have all the Linux tests set to run serial (while Windows/OSX run in parallel). So currently the Linux tests take anywhere from 30-40 mins. We had set Linux to serial because were were seeing all sorts of unexplained random failures. But we now believe that was due to multiple tests trying to access the user prefs. This change gives each test its own user prefs folder so internally (in another change) I can enable running Linux tests in parallel (reducing the time down to 5 mins).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooo - does that mean that the preflight checks will get faster? If so:

image

Copy link
Collaborator Author

@seando-adsk seando-adsk left a comment

Choose a reason for hiding this comment

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

This change will also allow us to run the tests in parallel on Linux - greatly reducing the time.

@kxl-adsk kxl-adsk added the unit test Related to unit tests (both python or c++) label Feb 9, 2021
Copy link
Contributor

@pmolodo pmolodo 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, in theory - we'll know once the tests finish!

Comment on lines +320 to +329
# Set a temporary folder path for the MAYA_APP_DIR in which the
# maya profile will be created.
# Note: replace bad chars in test_name with _.
string(REGEX REPLACE "[:<>\|]" "_" SANITIZED_TEST_NAME ${test_name})
set(MAYA_APP_TEMP_DIR "${CMAKE_BINARY_DIR}/test/Temporary/${SANITIZED_TEST_NAME}")
# Note: ${WORKING_DIR} can point to the source folder, so don't use it
# in any env var that will write files (such as MAYA_APP_DIR).
set_property(TEST "${test_name}" APPEND PROPERTY ENVIRONMENT
"MAYA_APP_DIR=${MAYA_APP_TEMP_DIR}")
file(MAKE_DIRECTORY ${MAYA_APP_TEMP_DIR})
Copy link
Contributor

@pmolodo pmolodo Feb 9, 2021

Choose a reason for hiding this comment

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

The problem we saw earlier wasn't direct test failures, per-se... but just that the tests ran over a timeout when run in the preflight context. I never had any test failures when running locally, or really noticed much of a time difference when running the tests with the MAYA_APP_DIR set... but apparently it made a difference in whatever environment pre-flight was running.

Which isn't to say that your changes won't fix whatever the cause of that slowdown was - maybe it was something related to running out of the WORKING_DIR? But just be on the watch-out for it, I guess!

Anyway, I'll assign the bot user to get the tests to run, so hopefully we'll know soon!

@seando-adsk seando-adsk added ready-for-merge Development process is finished, PR is ready for merge do-not-merge-yet Development is not finished, PR not ready for merge and removed ready-for-merge Development process is finished, PR is ready for merge labels Feb 10, 2021
@seando-adsk seando-adsk added ready-for-merge Development process is finished, PR is ready for merge and removed do-not-merge-yet Development is not finished, PR not ready for merge labels Feb 10, 2021
@kxl-adsk kxl-adsk merged commit bf3c9fb into dev Feb 11, 2021
@kxl-adsk kxl-adsk deleted the donnels/set_maya_app_dir_for_each_test branch February 11, 2021 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge Development process is finished, PR is ready for merge unit test Related to unit tests (both python or c++)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants