Skip to content

Fix enum idempotency #291

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 2 commits into from
Feb 9, 2024
Merged

Conversation

Clebam
Copy link
Contributor

@Clebam Clebam commented Feb 8, 2024

Summary

Linked to issue : #265
Fixes the fact that enum are not idempotent

Related Issues (if any)

#265

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.
  • Manually verified.

@Clebam Clebam requested a review from a team as a code owner February 8, 2024 13:30
@CLAassistant
Copy link

CLAassistant commented Feb 8, 2024

CLA assistant check
All committers have signed the CLA.

@jordanbreen28
Copy link
Contributor

@Clebam this is great! I know this has been a pain point for some users, myself included.
Could you address the rubocop errors?
I wouldn't worry about the Metrics cops - you can disable inline or add to a .rubocop_todo.yml, whatever you prefer.

@jordanbreen28 jordanbreen28 linked an issue Feb 8, 2024 that may be closed by this pull request
@Clebam Clebam force-pushed the fix_enum_idempotency branch from 3e5a370 to 95b9130 Compare February 8, 2024 14:58
@Clebam
Copy link
Contributor Author

Clebam commented Feb 9, 2024

Hi @jordanbreen28 , I'm a bit stuck on the last spec. I wonder if this is link to the spec itself. Do you have any insight on what this test is trying to perform ?

@jordanbreen28
Copy link
Contributor

hey @Clebam - sure! So it looks like the context is getting a message it isn't expecting, which in this case is your change to check the type.
If you add an extra expectation to context here

it should hopefully fix the error!

@Clebam
Copy link
Contributor Author

Clebam commented Feb 9, 2024

@jordanbreen28 I tried many things but I can't get rid of the error. I'm not that familiar with ruby so it's quite hard to grasp what is going wrong.

@jordanbreen28
Copy link
Contributor

@Clebam No problem, its all good! I was just about to check this pr out and try a few things myself as you tagged me. Let me see if I can spot the problem

@jordanbreen28
Copy link
Contributor

@Clebam altering the test case

it 'treats the manifest value as canonical' do
to the below worked for me - commit it and we will see what happens :)

            it 'treats the manifest value as canonical' do
              expect(context).to receive(:type).and_return(type)
              expect(type).to receive(:attributes).and_return({ dsc_property: { type: "Enum['Dword']" } })
              expect(canonicalized_resource.first[:dsc_property]).to eq('Dword')
            end

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9721f08) 91.83% compared to head (e2cc873) 91.96%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
+ Coverage   91.83%   91.96%   +0.12%     
==========================================
  Files           6        6              
  Lines         698      709      +11     
==========================================
+ Hits          641      652      +11     
  Misses         57       57              

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

Copy link
Contributor

@jordanbreen28 jordanbreen28 left a comment

Choose a reason for hiding this comment

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

and we're green! Great work @Clebam and thanks for your contribution!

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

Successfully merging this pull request may close these issues.

Canonicalisation of Enum[] is not idempotent
4 participants