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

[Code Quality]: Unit tests for yaml loader #324

Merged
merged 8 commits into from
Jan 27, 2023

Conversation

joshfried-aws
Copy link
Contributor

Issue #, if available:

Description of changes:

As part of #208, this PR increase test coverage. I have added some unit tests for the yaml loader. I also changed some of the rust code, applying clippy lints, and refactoring some of the code so its idiomatic.

In addition to these unit tests, this PR also adds some bug fixes I noticed along the way.

Bug Fixes

loader.rs

  • logical error when checking to see if a value was a float
  • logical error due to a difference in the way serde_yaml, and yaml_rust handled type refs (values with a !!)

Code Cleanup

eval_test.rs

  • applied clippy lints
    • mostly relating to stuff like assert_eq!(predicate, true). This is not needed, we can simply rewrite this as `assert!(predicate)

loader.rs

  • using lazy_static! i created static hashmaps/hashsets instead of matching on a bunch of different str types.
    • this makes the functions a lot easier to read imo
  • pulled some functions out of the impl block for the loader class, since they were stateless
  • because serde_yaml automatically converts a reference like !!float to tag:yaml.org,2002:float I changed the check on the suffix to look for any string that is prefixed with tag:yaml.org,2002:, and i changed the function that we call to match on tag:yaml.org,2002:float, tag:yaml.org,2002:int, tag:yaml.org,2002:null, etc... instead of how we did it before.
  • changed the way we manipulated values into bool/ints/floats. previously we were checking the value to see if every character was numeric, this breaks for floats. Now we parse ints first, if that fails we attempt to parse a float, if that fails we attempt to parse a bool, if that fails we default to a string value.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@joshfried-aws joshfried-aws merged commit 42d4f4e into aws-cloudformation:custom_writer Jan 27, 2023
joshfried-aws added a commit that referenced this pull request Jan 31, 2023
…n test framework, and adding initial tests for all commands (#325)

* Origin/custom writer (#319)

* Added build targets to release workflow

* Added build targets to release workflow

* Moved TARGETS env variable

* Removed JSON variable

* Added OS-specific runs

* Removed external dependency

* Added braces to target in path

* Reverting to OS-specific runs

* Fixed indent

* temp commit with temp fix for DI on execute method

* small fix + implementing poc for a test

* Added initial tests for verbose output validation

* Refactoring to add Writer across all commands

* Minor refactoring, removed unused imports

* rename from_utf8 to into_string due to clippy warning

* fixed bug from renaming from_utf8

Co-authored-by: Akshay Rane <raneaks@amazon.com>
Co-authored-by: Josh Fried <joshfri@amazon.com>

* Set all field in rules file to serialize with recursive singleton map  (#320)

* testing rust-fmt action with incorectly formatted file

* reverting previous commit

* testing rust-fmt action with incorectly formatted file

* reverting previous commit

* fixed serialization issues for enums

* cleanup

* Refactored resources and tests directory (#321)

Co-authored-by: Akshay Rane <raneaks@amazon.com>

* [Code Quality]: Improve test coverage (#322)

* testing rust-fmt action with incorectly formatted file

* reverting previous commit

* testing rust-fmt action with incorectly formatted file

* reverting previous commit

* create test runners for validate, parse-tree, and test. refactored tests in validate to use rstest + added new test cases. added parse-tree test cases

* removed unneeded cargo_test2 fn

* removing unused cargo_test fn

* rust fmt

* more parse-tree tests

* fixes as per comments

* rustfmt

* Custom writer (#323)

* Adding license info for indoc

* Added tests for migrate and rulegen, added constants for status codes, incorporated writer into test command

* Formatting changes

* Corrected typos in resource file paths

* Added license info for strip-ansi-escapes library

---------

Co-authored-by: Akshay Rane <raneaks@amazon.com>

* [Code Quality]: Unit tests for yaml loader  (#324)

* testing rust-fmt action with incorectly formatted file

* reverting previous commit

* testing rust-fmt action with incorectly formatted file

* reverting previous commit

* more parse-tree tests

* fixes as per comments

* added tests for laoder and fixed some bugs found while writing tests

* removed unneeded use

---------

Co-authored-by: Akshay Rane <aks.rane@gmail.com>
Co-authored-by: Akshay Rane <raneaks@amazon.com>
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.

None yet

2 participants