[WIP][FEEDBACK] Reduce test time by substantially reducing number of serialization tests. #10624

mjkkirschner · 2020-05-01T21:47:02Z

Purpose

https://jira.autodesk.com/browse/DYN-2666

While investigating the test job timeouts, I found that the serialization tests were a major portion of the testing time, and quite variable, as they require disk access 6 times each.
(open file, save file, open file, save file, save DS file, save results file)

In this PR I have simply limited the total number of files returned in the test generation function FindWorkspaces each returned workspace acts as a test case, but this function is accessed from multiple test classes for a total of approximately 5000 tests.

SerializationTests
WPFJsonSerializationTests
SerializationNonGUIDtests
SerializationCultureTests

After this change, the number becomes 1000~ tests total.

The time savings is quite large.
The complete install job completes in 1hour 30 minutes compared to 2:30 - 3:30 average.
The parallel job running on each PR completes in 11 minutes when run on the perf machine - compared to 30 minutes.

I am not sure how a reduction in the number of tests like this will effect coverage - the job is currently in progress. I will add that info when I have it. I am hopeful many of these tests are covering the same functionality... serialization.

Here are a few thoughts:

These serialization tests were used during the transition from xml to json files - that transition has completed, it's definitely still valuable to run the json tests over a subset of files (especially .xml files) in the repo - but it does not make sense to run these tests over the ever increasing collection of .json based files we already have! It will continue to grow and IMO does not provide much as a test besides wasting time.

These tests also used to produce output files that could be used to compare with previous executions - this is quite useful for other implementations of DesignScript, but has been turned off for some time.

If we need this functionality for an ever increasing set of files we should run it as a separate job which produces this suite of executions and not tie to our tests which should be fast!

here are some other proposals to balance testing coverage vs speed.

disregard all files inside FindWorkspaces which are not xml files.
create a handpicked list of files instead of finding the first n files in the repo.
disable the SerializationTests - but keep the JsonSerialization tests - these duplicate much of the testing in the SerializationTests - as they are a super set.

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.

mmisol · 2020-05-01T21:55:30Z

Great find @mjkkirschner ! Thinking about possible ways to deal with this, we could also move these tests to another job that runs on a fixed schedule, maybe daily.

mjkkirschner · 2020-05-02T02:04:13Z

@smangarole I'm not sure - but from looking at the latest sq coverage activity at 7:38pm it appears for all these tests not run - we only lost .3% coverage... that sounds like a good tradeoff for the speedup - and I bet we can make it back by targeting those lines specifically or with specific graphs more efficiently.

I'm not 100% though on if the job you ran made it into the report or where I can go to see that independently? Is that possible? I want to verify these are the right numbers.
https://sq.autodesk.com/project/activity?graph=coverage&id=DynamoCA&selected_date=2020-05-01T23%3A38%3A21%2B0000

gregmarr · 2020-05-04T15:15:33Z

I think we could definitely do this as a daily scheduled job.

aparajit-pratap · 2020-05-05T13:49:45Z

These tests also used to produce output files that could be used to compare with previous executions - this is quite useful for other implementations of DesignScript, but has been turned off for some time.

@mjkkirschner what do you mean by this? Are the serialization tests comparing results from XML and JSON files? If so, what is tested with new JSON files that do not have XML versions?

QilongTang · 2020-05-05T14:11:20Z

src/DynamoCore/Graph/Workspaces/SerializationConverters.cs

@@ -1076,9 +1077,9 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist
            Guid deterministicGuid;
            if (!Guid.TryParse(obj.Value<string>(), out deterministicGuid))
            {
-                Console.WriteLine("the id was not a guid, converting to a guid");
+                Debug.WriteLine("the id was not a guid, converting to a guid");


@mjkkirschner Can you remind me the consequence of this change, if I remember correctly this will reduce the console log size we see in Jenkins correct?

yes, we should not get logs filled with random guids - these were useful when we were creating this functionality, but not so much anymore.

QilongTang · 2020-05-05T14:11:46Z

test/DynamoCoreWpfTests/SerializationTests.cs

@@ -1006,7 +1009,7 @@ public object[] FindWorkspaces()
            var di = new DirectoryInfo(TestDirectory);
            var fis = new string[] { "*.dyn", "*.dyf" }
            .SelectMany(i => di.GetFiles(i, SearchOption.AllDirectories));
-            return fis.Select(fi => fi.FullName).ToArray();
+            return fis.Select(fi => fi.FullName).Take(MAXNUM_SERIALIZATIONTESTS_TOEXECUTE).ToArray();


👍 I am supportive

gregmarr · 2020-05-05T14:48:23Z

@aparajit-pratap I'll point you to internal resources on this question.

QilongTang · 2020-05-05T18:18:10Z

LGTM

* add namespace conflict tests for short name replacer and node to code (#10611) * Register custom node before package load reset (#10591) This fixes a problem where existing custom nodes in the home workspace became unresolved after a package that contained binaries was loaded. The cause of the problem was that the compiled function was not available at the time of the execution after a VM reset. Now the data is registered on package load, by queueing it in pendingCustomNodeSyncData. This results in a CompileCustomNodeAsyncTask being scheduled before the update of the home workspace graph takes place. * Increase coverage of the Core folder (#10609) * Add a test for the CrashPromptArgs class * Update DynamoCoreTests.csproj * Added NotificationObject tests * Updated the test, NotificationObject coverage at 100% * Changed some access modifiers * Added coverage for Updates/BinaryVersion * Added comments and fixed names * Revert "Added comments and fixed names" This reverts commit 42cd024. * Revert "Added coverage for Updates/BinaryVersion" This reverts commit 679deca. * Increased coverage in CrashPromptArgs * Added some Core coverage * DYN-2560 - Increase the code coverage: DynamoCore/Models Folder First Part (#10612) * DYN-2560 - Increase the code coverage: DynamoCore/Models Folder I started adding just one test method TestOnRequestDispatcherBeginInvoke() for testing the DynamoModelEvents class. * DYN-2560 - Adding test cases for DynamoModelEvents Adding test cases for DynamoModelEvents * DYN-2560 - Adding test cases for DynamoModelEvents I added all the test cases for the all events in the DynamoModelEvent class, i just need to fix the last 6 of them. * DYN-2560 - Adding test cases for DynamoModelEventsArgs I added several test cases for the classes inside the DynamoModelEventsArgs.cs file. ZoomEventArgs TaskDialogEventArgs EvaluationCompletedEventArgs DynamoModelUpdateArgs FunctionNamePromptEventArgs PresetsNamePromptEventArgs ViewOperationEventArgs PointEventArgs WorkspaceEventArgs ModelEventArgs * DYN-2560 - Code Review Comments Based in the comment done by Aaron in the GitHub pull request, I added more description comments for the method TestTaskDialogEventArgs() and also I added comments for a local function * DYN-2560 - Code Review Comments 2 There was a spelling error in two methods for the word "Internally", then I fixed this error in the two places. * Python Engine Enum (#10618) * Cherrypick * Comments * Add unit test * Comments * Handle runtime table gaps on code block deletion (#10605) When the runtime table are built there is an implicit assumption that the code block ids are consecutive. However, that is not always the case, as the deletion of a procedure causes the deletion of its child code blocks, which may generate gaps in the id numbering. In order to make the code resiliant to these gaps, the runtime tables are sized based on the largest code block id, rather than in the amount of code blocks. * Validate ASM installations before loading (#10621) The check for the specific assemblies tbb.dll and tbbmalloc.dll is generalized to a full file list validation of detected ASM locations. This way, Dynamo is guarded against any incomplete/unusual ASM binary folders that other applications might include. The lists of files for each version were taken from LibG. They cannot be reused from LibG without involving major changes in the preloader, so the lists should be kept in sync as new major release of ASM occur. * SQ bug fix (#10622) * (1) Null reference bug fix from SQ dashboard * Add support for debug modes (#10603) * Add support for debug modes * Increase coverage of the Updates folder (#10628) * Add a test for the CrashPromptArgs class * Update DynamoCoreTests.csproj * Added NotificationObject tests * Updated the test, NotificationObject coverage at 100% * Changed some access modifiers * Added coverage for Updates/BinaryVersion * Added comments and fixed names * Added asserts for happy path, changed namespace and deleted a console function * Removed using * Revert "Removed using" This reverts commit 022823d. * Removed a change that should not be there * Added coverage for Updates folder * Tests for CPython3 Engine as well as for IronPython. * [WIP][FEEDBACK] Reduce test time by substantially reducing number of serialization tests. (#10624) * reduce number of serialization tests by factor 3~ * reduce wpf json serialization tests * Handle missing instance calling method statically (#10630) A code block node calling an instance method in its static form would make Dynamo crash if the instance was not provided. An example of this would be calling 'Curve.Patch();'. The cause of the issue was that the default argument was ultimately tried to be interpreted as a pointer. By avoiding that wrong conversion the engine is now able to surface the real problem as a human-readable warning. * Python3 Selection Under Debug Modes (#10629) * Cherrypick Python3 changes * Cleanup * Use Debug Modes * CleanUp * Rename Function * Clean Up * Do not use anouymous function as handler * Revert newer language change * Add adp analytics to Dynamo (#10576) * add ADPTracker register * Fix non-array item search for dot operation (#10633) When an array is passed to the dot operation, it needs to get an item to determine the actual class being processed. This was done in ArrayUtils.GetFirstNonArrayStackValue, but the function only checked the first item of the array and its descendants. This caused the dot operation to failed when passed an array that contained an empty array as its first item. In order to fix the problem, the function was changed to check for non-array items in the entire array. The function should stop as soon as the first non-array item is found. * Addressing some comments * some more comments * Update AssemblyInfo.cs * Update AssemblyInfo.cs * Marking the python node as modified when its engine property is modified Also updated some tests. * Remove unwanted check. * changes to test * Update AssemblySharedInfo.cs * Using python engine's AcquireLock to avoid deadlock. The deadlock was happening only when multiple PythonEngine's are initialized on a thread from 2 different test fixtures in the same run. The main difference is calling this function PythonEngine.BeginAllowThreads(). Also we do not want to initialze the python engine if it is already initialized. Co-authored-by: aparajit-pratap <[email protected]> Co-authored-by: Martin Misol Monzo <[email protected]> Co-authored-by: Bruno Yorda <[email protected]> Co-authored-by: Roberto T <[email protected]> Co-authored-by: Aaron (Qilong) <[email protected]> Co-authored-by: Ashish Aggarwal <[email protected]> Co-authored-by: Tibi <[email protected]> Co-authored-by: Michael Kirschner <[email protected]>

mjkkirschner added 2 commits April 30, 2020 17:56

reduce number of serialization tests by factor 3~

b9cb0ea

reduce wpf json serialization tests

22d55c7

mjkkirschner changed the title ~~[WIP][FEEDBACK]Reduce test time by substantially reducing number of serialization tests.~~ [WIP][FEEDBACK] Reduce test time by substantially reducing number of serialization tests. May 1, 2020

QilongTang reviewed May 5, 2020

View reviewed changes

QilongTang added the LGTM Looks good to me label May 5, 2020

mjkkirschner merged commit 7391433 into master May 5, 2020

mjkkirschner deleted the reduceTestTime branch May 5, 2020 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP][FEEDBACK] Reduce test time by substantially reducing number of serialization tests. #10624

[WIP][FEEDBACK] Reduce test time by substantially reducing number of serialization tests. #10624

mjkkirschner commented May 1, 2020 •

edited

Loading

mmisol commented May 1, 2020

mjkkirschner commented May 2, 2020 •

edited

Loading

gregmarr commented May 4, 2020

aparajit-pratap commented May 5, 2020

QilongTang May 5, 2020

mjkkirschner May 5, 2020

QilongTang May 5, 2020

gregmarr commented May 5, 2020

QilongTang commented May 5, 2020

[WIP][FEEDBACK] Reduce test time by substantially reducing number of serialization tests. #10624

[WIP][FEEDBACK] Reduce test time by substantially reducing number of serialization tests. #10624

Conversation

mjkkirschner commented May 1, 2020 • edited Loading

Purpose

Declarations

mmisol commented May 1, 2020

mjkkirschner commented May 2, 2020 • edited Loading

gregmarr commented May 4, 2020

aparajit-pratap commented May 5, 2020

QilongTang May 5, 2020

Choose a reason for hiding this comment

mjkkirschner May 5, 2020

Choose a reason for hiding this comment

QilongTang May 5, 2020

Choose a reason for hiding this comment

gregmarr commented May 5, 2020

QilongTang commented May 5, 2020

mjkkirschner commented May 1, 2020 •

edited

Loading

mjkkirschner commented May 2, 2020 •

edited

Loading