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

Fixes testUpdateBranchGrayRulesWithUpdateOnce #4599

Merged

Conversation

ZhewenFu
Copy link
Contributor

What's the purpose of this PR

Fix test testUpdateBranchGrayRulesWithUpdateOnce

Which issue(s) this PR fixes:

Fixes the issue that causes flaky tests. The current test may lead to inconsistent ordering of stringified JSON objects and the test may fail sometimes. To fix the test and make it consistent, I select all the elements in the list and see whether the release history contains those elements. In this case, the ordering of the elements doesn't matter and won't cause failure.

Brief changelog

Added containRule function.

@github-actions
Copy link

github-actions bot commented Oct 10, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@ZhewenFu
Copy link
Contributor Author

ZhewenFu commented Oct 10, 2022

Sorry I closed my previous PR by accident. The reason I didn't use GrayReleaseRuleItemTransformer#batchTransformFromJSON is once we transform it into object, the object's toString() method is not suitable for the contains method and still requires us to perform string manipulation.

@zhewenfufufu
Copy link
Contributor

I have read the CLA Document and I hereby sign the CLA

@zhewenfufufu
Copy link
Contributor

recheck

@codecov
Copy link

codecov bot commented Oct 10, 2022

Codecov Report

Merging #4599 (0473cd6) into master (75ae5cc) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #4599   +/-   ##
=========================================
  Coverage     47.23%   47.23%           
  Complexity     1649     1649           
=========================================
  Files           347      347           
  Lines         10631    10631           
  Branches       1053     1053           
=========================================
  Hits           5022     5022           
  Misses         5306     5306           
  Partials        303      303           
Impacted Files Coverage Δ
...ervice/service/ReleaseMessageServiceWithCache.java 85.88% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@nobodyiam nobodyiam force-pushed the testUpdateBranchGrayRulesWithUpdateOnce branch from 607f035 to e0499a8 Compare October 23, 2022 10:31
@nobodyiam
Copy link
Member

The unit test failed, which seems related to the line of code

Set<GrayReleaseRuleItemDTO> contextRules = GrayReleaseRuleItemTransformer.batchTransformFromJSON(context.substring(context.indexOf("["),context.lastIndexOf("]")+1));

@ZhewenFu
Copy link
Contributor Author

The unit test failed, which seems related to the line of code

Set<GrayReleaseRuleItemDTO> contextRules = GrayReleaseRuleItemTransformer.batchTransformFromJSON(context.substring(context.indexOf("["),context.lastIndexOf("]")+1));

Yeah that seems to be where the problem lies. I have changed it to transfer the context to a map struct to avoid hardcoding. Now it should work fine.

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@nobodyiam nobodyiam merged commit d849c02 into apolloconfig:master Oct 28, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 2022
@nobodyiam nobodyiam added this to the 2.1.0 milestone Feb 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants