Skip to content

Unit test + fix for state file handling in #115 #124

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.

Sign up for GitHub

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

Jump to bottom
Open
malmond77 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Unit test + fix for state file handling in #115 #124

malmond77 wants to merge 2 commits into from

Conversation

@malmond77
Copy link
Contributor

@malmond77 malmond77 commented Aug 19, 2019

Two parts:

  1. Demonstrate the bug in a unit test
  2. Fix the bug

@malmond77
Unit test that demonstrates bug in state update logic
bd61ef7 
This commit demonstrates a bug in state handling. Every modification involves a read, modify, write. This unit test mocks out just enough to show that a state file with ssl -> true is read correctly, changed, then produces an invalid json file with two entries in the hash with the same key. This is the key issue as reported in facebook#115.

```
[malmond@devvm1846.vll1 ~/local/taste-tester]
/opt/chefdk/embedded/bin/rspec .
F

Failures:

  1) TasteTester::State should serialize changes correctly
     Failure/Error: expect(@buffer.string).to eq('{"ssl":false}')

       expected: "{\"ssl\":false}"
            got: "{\"ssl\":true,\"ssl\":false}"

       (compared using ==)
     # ./spec/taste-tester_spec.rb:20:in `block (2 levels) in <top
     # (required)>'

Finished in 0.01544 seconds (files took 0.27334 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/taste-tester_spec.rb:6 # TasteTester::State should serialize changes correctly
```

Signed-off-by: Matthew Almond <malmond@fb.com>
@malmond77
Replace mash usage with simpler .to_s coercion on key on writes
e3746fb 
This is to fix the issue reported in facebook#115.

It is possible to modify `state.rb` to use Mash in a way that does merge keys properly. However this is more complex than neccessary. The existing code already uses `.to_s` on reads. Given a choice between the two approaches, I am chosing to keep the code simpler and remove the unneccessary import.

Signed-off-by: Matthew Almond <malmond@fb.com>
jaymzh
jaymzh requested changes Aug 19, 2019
View reviewed changes
Copy link
Collaborator

@jaymzh jaymzh left a comment

Choose a reason for hiding this comment

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

JSON forces all keys to strings.
Mash forces all keys to be consistent.

The existing code fixes the bug I opened.... and I don't see this showing any legitimate bug, it's just doing the same work manually.

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

Reviewers

@jaymzh jaymzh jaymzh requested changes

@waffle2k waffle2k Awaiting requested review from waffle2k

@davide125 davide125 Awaiting requested review from davide125

@joshuamiller01 joshuamiller01 Awaiting requested review from joshuamiller01

@NaomiReeves NaomiReeves Awaiting requested review from NaomiReeves

+1 more reviewer

@nikitakoshikov nikitakoshikov nikitakoshikov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@malmond77 @jaymzh @nikitakoshikov @facebook-github-bot