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

ATOMS - Automatic Trivial Oxidized Model Spec #3361

Merged
merged 48 commits into from
Jan 9, 2025
Merged

ATOMS - Automatic Trivial Oxidized Model Spec #3361

merged 48 commits into from
Jan 9, 2025

Conversation

robertcheramy
Copy link
Collaborator

Pre-Request Checklist

  • Passes rubocop code analysis (try rubocop --auto-correct)
  • Tests added or adapted (try rake test)
  • Changes are reflected in the documentation
  • User-visible changes appended to CHANGELOG.md

Description

This PR introduces ATOMS (Automatic Trivial Oxidized Model Spec), which greatly simplifies model unit tests. Most of the code is from @ytti, @robertcheramy documented it and updated device2yaml.rb.

Note: the PR will be squashed & merged.

ytti and others added 30 commits December 19, 2024 10:32
See if it makes sense to simplify simple case of model testing
a) output is plain ascii, not yaml (TODO, it is yaml, and uses
yaml_interpolate)
b) we no longer do n^2 to figure out files, much more streamlined way to
build test
Will make it easier to instantitate ATOMS test elsehwere too
This allows us to report why some test are skipped, for example if we
failed to load a data file
Flat list of tests instead of hash of tests with type key
Simplify creating the result object
No point requiring it in each model test
Removes the need to always instantiate test first in the model specs
New names make it more convenient to choose which tests to run, eg:

```
╰─ ruby -Ilib:test spec/model/model_atoms_spec.rb --name "/output:ios/" --verbose
Run options: --name /output:ios/ --verbose --seed 41151

ATOMS tests#test_0017_output:ios:C9200L-24P-4G_17.09.04a has expected output = 1.01 s = .
ATOMS tests#test_0019_output:ios:asr920_16.8.1b has expected output = 0.73 s = .
ATOMS tests#test_0018_output:ios:C9800-L-F-K9_17.06.05 has expected output = 0.72 s = .

Finished in 2.465471s, 1.2168 runs/s, 3.6504 assertions/s.

3 runs, 9 assertions, 0 failures, 0 errors, 0 skips
Coverage report generated for RSpec to /home/ytti/git/oxidized/coverage/coverage.xml. 658 / 1146 LOC (57.42%) covered
Coverage report generated for RSpec to /home/ytti/git/oxidized/coverage.
Line Coverage: 57.42% (658 / 1146)
```
I did comment atoms.rb while reading & understanding it. Nice peace of
code!
must_match is meant for a regexp, but test.output is plain text.
When we use must_equal, we get a nice diff of the differences when the
test fails.
"---" at the beginning of a YAML file is optional, and we have files
with and files without ---.
This commit makes all files comply to the decision (@ytti +
@robertcheramy) not to use ---.
robertcheramy and others added 16 commits December 30, 2024 15:33
- Removing garderos_spec.rb as is it covered with ATOMS
- Documenting atoms.rb further
- We may need a fail_with_expect: but I would only implement it when we
need it.
Avoids useless tests if previous tests about output would fail anyhow.
Intended to remove 'rescue nil' but only removed 'l'.
- include de command sets into device2yaml.rb / This could be solved in
a better way
- move examples/device-simulation/README.md to /docs/DeviceSimulation.md
- adapt the documentation to the new path
- remove examples/device-simulation
- the YAML Simulation files do not have an oxidized_output: section
anymore
- spec/model/README.md is still to be updated
k = ''
loop do
 k << 'a'
end

Wont work in future, because k is string literal, therefore frozen/immutable, and String#<< method tries to mutate it.

Fixes are
  k = String.new
  k = '' + ''   # expression is not string literal
  k = ''.dup
From ruby 3.5.0 forward ostruct won't be available by standard
- Preparing merge into master (some polishing is still needed)
- Adapt new tests from master (fsos and cumulus prompts) to ATOMS
- Place the yaml files under spec/model/data
- Remove command sets from master
- The cumulus tests needs further development, which will be done after
the merge, so the tests are skipped for now
@robertcheramy robertcheramy self-assigned this Jan 9, 2025
@robertcheramy robertcheramy requested a review from ytti January 9, 2025 14:23
@robertcheramy
Copy link
Collaborator Author

@ytti - I'm ready to merge. Do you want to make a last review?

@ytti
Copy link
Owner

ytti commented Jan 9, 2025

Go for it and thank you.

@robertcheramy robertcheramy removed the request for review from ytti January 9, 2025 14:26
@robertcheramy robertcheramy merged commit 0e19aed into master Jan 9, 2025
12 checks passed
@robertcheramy robertcheramy deleted the ATOMS branch January 9, 2025 14:29
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.

2 participants