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

Fixed failure in "TestTaskQueuePreProcessing00" #4462

Merged
merged 2 commits into from
May 13, 2015

Conversation

Benglin
Copy link
Contributor

@Benglin Benglin commented May 13, 2015

Purpose

This pull request is meant to fix the following test case:

SchedulerIntegrationTests.TestTaskQueuePreProcessing00

The failure was introduced due to a number of different behaviors in both the system and tests:

  1. Add task merging for graph updates
  2. Run all recorded test case in auto mode

Declarations

Check these iff you believe they are true

  • The code base is in a better state after this PR
  • The level of testing this PR includes is appropriate

Reviewers

Hi @aparajit-pratap, this is related to the failing test case that you were debugging this morning. Please have a look, thanks!

FYIs

Hi @lukechurch and @ke-yu, just FYI. The new UpdateGraphAsyncTask merging behavior definitely has changed the expected behavior of this test (and one of the two tasks was dropped as expected).

@Benglin Benglin added the PTAL Please Take A Look 👀 label May 13, 2015
@@ -25,7 +25,7 @@ internal override TaskPriority Priority
get { return TaskPriority.AboveNormal; }
}

internal IEnumerable<NodeModel> ModifiedNodes { get; private set; }
public IEnumerable<NodeModel> ModifiedNodes { get; protected set; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need this member to be filled up by the derived class FakeUpdateGraphAsyncTask so that it won't be null when the base method UpdateGraphAsyncTask.CanMergeWithCore is called (this data member will never be null in the usual UpdateGraphAsyncTask).

@aparajit-pratap
Copy link
Contributor

@Benglin to me it looks like @lukechurch has basically merged multiple UpdateGraphAsyncTasks that are essentially executing the same set of nodes into one so in this test case instead of expecting two of them (for the same set of nodes) we can now expect just one of them after the queue is processed. If so, it looks good to me.

@Benglin
Copy link
Contributor Author

Benglin commented May 13, 2015

Spot on, @aparajit-pratap! That's exactly why @lukechurch's original PR was created. Merging this in to stabilize the build, thanks again!

Benglin added a commit that referenced this pull request May 13, 2015
Fixed failure in "TestTaskQueuePreProcessing00"
@Benglin Benglin merged commit e39c231 into DynamoDS:master May 13, 2015
@Benglin Benglin deleted the UnitTestFix branch May 13, 2015 08:52
@Benglin Benglin added LGTM and removed PTAL Please Take A Look 👀 labels May 13, 2015
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.

2 participants