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

CCVAR: Fixed Same Attribute not updating correctly #3275

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

vabs95
Copy link

@vabs95 vabs95 commented Feb 26, 2024

Defect: CCVAR Same Attribute doesn't update all placeholders in the value.

Uses the original attribute value and only updates the last placeholder value leaving the other placeholders as it is.

Example:

Attr "title=((custom_prefix)) ((custom_title))"

Expected: "title=Prefix Title"

Actual: "title=((custom_prefix)) Title"

Solution: Update the currentAttr Value with new attrValue when looping placeholders.

Copy link

codecov bot commented Feb 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.45%. Comparing base (7676ed5) to head (9d78151).
Report is 19 commits behind head on master.

Current head 9d78151 differs from pull request most recent head 2000b24

Please upload reports for the commit 2000b24 to get more accurate results.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3275      +/-   ##
============================================
+ Coverage     55.34%   55.45%   +0.11%     
+ Complexity     5543     5506      -37     
============================================
  Files           728      721       -7     
  Lines         29705    29535     -170     
  Branches       3875     3840      -35     
============================================
- Hits          16440    16379      -61     
+ Misses        11722    11630      -92     
+ Partials       1543     1526      -17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vabs95 vabs95 force-pushed the defect/ccvar/same-attr-not-updating branch from 1a61329 to 6b37a21 Compare February 26, 2024 13:14
@vabs95
Copy link
Author

vabs95 commented Feb 26, 2024

@kwin @csaboka @davidjgonzalez I added this fix, can you please review.

Also, I added test case but it still failing. Can you please tell me what am I missing here?

@csaboka
Copy link
Contributor

csaboka commented Feb 26, 2024

@vabs95 : JUnit 4 tests are not running at the moment, I'm trying to fix that in #3273 . Since the test you are trying to extend uses JUnit 4 tools, it doesn't run at all and won't contribute to coverage no matter what you change.

You can either wait until #3273 is merged, or try rewriting the test class to JUnit 5.

@vabs95
Copy link
Author

vabs95 commented Mar 6, 2024

@csaboka Can you please review it now?

@csaboka
Copy link
Contributor

csaboka commented Mar 6, 2024

@vabs95 : I'm not a project owner, just a minor contributor to the project. I'm not familiar with the code you touched, but even if I were, my approval wouldn't be enough to allow merging this.

@vabs95
Copy link
Author

vabs95 commented Mar 7, 2024

@kwin can you please review it ?

@vabs95 vabs95 changed the title CCVAR: Fixed Same Attribute not updating correctly. CCVAR: Fixed Same Attribute not updating correctly Jul 8, 2024
@vabs95
Copy link
Author

vabs95 commented Jul 8, 2024

@davidjgonzalez can you please re-run the checks?

@vabs95
Copy link
Author

vabs95 commented Aug 5, 2024

hi @davidjgonzalez can you please merge this now? It has been pending for a long time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants