Skip to content

Update unit tests and simplify logic for dynamic update #2237

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

Merged
merged 9 commits into from
Mar 9, 2021

Conversation

jshum2479
Copy link
Member

Russ and I worked on adding unit tests and simplify the makeright logic for dynamic update.

@@ -1520,6 +1590,13 @@ public void whenPodCreated_createPodWithOwnerReference() {
assertThat(getCreatedPod().getMetadata().getOwnerReferences(), contains(expectedReference));
}

// TODO: add tests for dynamic updates
Copy link
Member

Choose a reason for hiding this comment

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

How hard would it be to create these tests? I'm not a fan of leaving a TODO in the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually the tests are there, removing TODO

@jshum2479
Copy link
Member Author

There is regression in the branch, withdrawing now.

@jshum2479 jshum2479 closed this Mar 2, 2021
@jshum2479
Copy link
Member Author

I found the problem, actually it's a bug rather than a regression in mii v1 model diff code

@jshum2479 jshum2479 reopened this Mar 3, 2021
@rjeberhard
Copy link
Member

@jshum2479, what's the status of this change? Did you resolve the regression?

@jshum2479
Copy link
Member Author

@rjeberhard Actually, it's not a regression, I found a bug in v1 and fixed that. It should be good to go

@rjeberhard rjeberhard merged commit 30aef63 into develop Mar 9, 2021
@rjeberhard rjeberhard deleted the todo-unittest-dynamicupdate branch January 31, 2022 14:10
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.

3 participants