-
Notifications
You must be signed in to change notification settings - Fork 475
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
POC/DRAFT: Add transforms #576
Conversation
Hi there, I'm interested in having a fix for #483 and I would strongly prefer contributing to upstream istead of mainating my own fork. If I understand correctly, this MR is a potential fix: is there any way I could assist you in finishing it? |
This PR is a potential fix in that it allows direct string matching of the command output. Rather than only limiting it to the current pattern matcher logic. Here's an example with some work from this PR: command:
test_exec:
exec: 'echo -e "This is an example output of a command\nGoss will print this in full when using the new string matcher\nas opposed to using the pattern matcher"'
exit-status: 0
stdout:
- This is an example output of a command, broken
- Goss will print this in full when using the new string matcher
- as opposed to using the pattern matcher
stderr: []
timeout: 10000
test_exec2:
exec: 'echo -e "This is an example output of a command\nGoss will print this in full when using the new string matcher\nas opposed to using the pattern matcher"'
exit-status: 0
stdout: |
This is an example output of a command, broken
Goss will print this in full when using the new string matcher
as opposed to using the pattern matcher
stderr: []
timeout: 10000 notice test_exec2 is testing stdout against a string rather than an array. The output of running this in the new version of goss will be:
This PR will be a pretty major refactor of how goss handles pattern matching and how results are output. It is a breaking change so it will be in goss v0.4.0.. and since v0.4.0 will be a breaking change, there's a few other breaking changes that I would like to have in there prior to release. Here's my todo.md (and progress) for v0.4.0: # TODO:
[X] Find way to get private unexported values from gomega matchers
[X] Implement gomega matcher for validate contains
[X] Remove old validatecontains code
[X] implement gjson transform
[X] Add equal matcher
[X] Use benumerically as defualt numbers
[X] Add toNumeric transformer
[-] create new testresult object
[x] Move all matcher data outside of matchers and into testresult object
[ ] Move all formatting related to testresult into outputs/
[-] Implement clean short failure messages for matchers
[ ] Check have-key message with matcher
[ ] Write docs
[ ] github.com/sanity-io/litter?
[ ] And matcher indent is funky/incorrect
[ ] get rid of have-key-with-value or dry out have-key-with-value?
# optional
[ ] catch .(string) type errors in gomega.go (if input is not controlled)
[ ] Implement have-patterns for array
[ ] all matcher potentially
[ ] investigate why string diff does not trigger for equal matcher
[ ] register matchers using init?
# Other v0.4.0 stories, since they potentially introduce breaking changes
[ ] psutil vs go-ps, possibly switch to the former
[ ] process, comm vs args. Can we match on either or with the psutil library?
[ ] rpm expand full version
[ ] rpm/deb version matchers? create generic generic version of semver.Range that can be used with a version interface. Create wrappers around 3rd party rpm/deb comparison libraries to honor version interface Let me know if there's something in there that you're interested in helping out with. If I had to guess I'm about ~60% done with this PR. However, I haven't even started on any of the other v0.4.0 work. |
I'll look into these early next week and will either do something or ask more questions then. Thank you for the detailed answer! |
Awesome, please reach out to me before putting in work/time, I want to avoid a situation where you do work I already completed. Also, is the string matcher change what you're looking for with regards to #483? The reason it can't be done with patterns is because goss reads a file line by line. This is apparent if you run |
I don't know yet how I would approach as the only thing I know for now is that delivering more information to the affected asset owner is a necessity. If I understand you correctly and it will work with text match but not with regexp, I am fine with it as it's infinitely better than the current situation which doesn't seem to have a workaround (at least I'm not aware of it). I think I'll start with looking into this PR and figuring out if I can help with it, and then going trough the list and pinging you in PRs\issues. |
…t as omegaMatcher
My apologies for the delay, I don't know if I'll have time to look into this as I got a lot of other stuff on hands. @nloginov is interested in having this in place as well and will likely try to help you at some point in time. |
@petemounce Tagging you on here since you are creating a bunch of PRs currently. This is a major work stream that I'm working on for goss v4. I would love your feedback on it. The manual.md changes and this file explain the differences: I will most likely create a v4 branch, this + a few other PRs will be merged into it and hopefully I can get v4 out the door within a month. This is a fairly large re-write and may impact some of the PRs you have out there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petemounce Tagging you on here since you are creating a bunch of PRs currently.
This is a major work stream that I'm working on for goss v4. I would love your feedback on it.
The manual.md changes and this file explain the differences:
examples/goss_awesome_gomega.yamlI will most likely create a v4 branch, this + a few other PRs will be merged into it and hopefully I can get v4 out the door within a month. This is a fairly large re-write and may impact some of the PRs you have out there.
I've reviewed just the docs in this pass; this is a pretty massive PR to feed back on. Is there any possibility of slicing it up into smaller chunks?
What kinds of feedback are you after? UX/requirements/design/implementation/coding style/tests/something else?
If I've understood the intent - this is so that inputs can be converted into the appropriate types to allow assertion with a richer set of matchers. Is that right?
- contain-element: "foo" | ||
- not: {contain-element: "bar"} | ||
- contain-element: {match-regexp: "[Gg]oss"} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this a typo on your end? github UI isn't clear on what this change actually is. Looks to be adding a -
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's removing a newline - sorry, nit-pick on my part! I have pre-commit
set up to automatically do things like that.
KernelParam: net.core.somaxconn: value: | ||
Expected | ||
128 | ||
to be > | ||
200 | ||
the transform chain was | ||
[{"to-numeric":{}}] | ||
|
||
Total Duration: 0.001s | ||
Count: 1, Failed: 1, Skipped: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a user, I would find this easier to interpret if
- the front of the messaging included the input value and type
- the end of the messaging showed the desired output type
So, sketching something like this?
KernelParam: net.core.somaxconn: value: | |
Expected | |
128 | |
to be > | |
200 | |
the transform chain was | |
[{"to-numeric":{}}] | |
Total Duration: 0.001s | |
Count: 1, Failed: 1, Skipped: 0 | |
KernelParam: net.core.somaxconn: value: | |
Expected | |
(string) 128 | |
to be > | |
(numeric) 200 | |
the transform chain was | |
[{"to-numeric":{ input: "128" }}] | |
Total Duration: 0.001s | |
Count: 1, Failed: 0, Skipped: 0, Error: 1 |
I put the types ahead of the values since the width of the type-string-names is predictable, and then the lines can be padded out to the longest one so that the values are all going to appear starting from the same column, making them easier to eyeball.
I added Error: 1
because it feels to me that a failure to transform is an internal error as opposed to a failed assertion. That struck me as something I'd like to know separately, since it means there's something wrong in how I've defined my assertions, rather than a for-sure assertion-failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suggestion accidentally also (I think?) removes the need for the -o include_raw
, since it includes the same information with no extra output lines needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gonna defer this conversation to regular github comments to make it easier to track. I'll detail the different use-cases there.
value: {gt: 200} | ||
``` | ||
|
||
When a transformed test fails, it will detail the transformers used, the `-o include_raw` option can be used to include the raw, untransformed attribute value: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means the tests would need to be re-run to get that extra information. As a user, I would prefer that if a test fails, all of the possible debugging information were given to me so I can have it up-front. Not doing that
- in the best case, costs me the time to re-run the tests
- but this is harder on machines that are remote
- in the worst case, costs me a lot of time to re-run the tests multiple times in the event of chasing an intermittent failure (much more likely in machine state testing, and more likely again if the test touches an external dependency like
addr
,http
etc).
|
||
* `'55'` - Checks that the numeric is "55" when converted to string | ||
* `have-prefix: pre` - Checks if string starts with "pre" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works because the default type in YAML is a string, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean have-prefix: pre
vs have-prefix: "pre"
if so, yes.. that's yaml-ism.
#### Array matchers | ||
|
||
These will convert the system attribute to an array prior to matching. Strings are split on "\n" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, this looks like -
being suggested as an addition.. is that correct?
* `contain-elements: [matcher, ...]` - checks if the array is a superset of the provided matchers | ||
* `[matcher, ...]` - same as above | ||
* `equal: [value, ...]` - Checks if the array is exactly equal to provided array | ||
* `consist-of: [matcher, ...]` - Checks if the array consists of the provided matchers (order does not matter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it to late to rename consist-of
to satisfies
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consist-of has been around since 2016. Maybe as part of 4 or a 4.X release I can add other aliases, but I prefer not breaking the old interface.
The verbiage was taken from the gomega project which goss relies on. https://onsi.github.io/gomega/#consistofelement-interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, gotcha. Yeah, definitely not worth a breaking change. If it's possible easily to alias, that might be nice, but ... not that nice.
* `contain-element: matcher` - Checks if the array contains an element that passes the matcher | ||
* `contain-elements: [matcher, ...]` - checks if the array is a superset of the provided matchers | ||
* `[matcher, ...]` - same as above | ||
* `equal: [value, ...]` - Checks if the array is exactly equal to provided array | ||
* `consist-of: [matcher, ...]` - Checks if the array consists of the provided matchers (order does not matter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't, from this and the example below, figured out where I would use contain-element
vs consist-of
. I find it difficult to understand partly because both of them allow matchers to be present.
I can think of wanting to:
- assert that an array contains particular elements/matches (without caring about ordering)
- assert that an array contains a sequence of particular elements/matches (e.g. contains a then any-single-element then b, but that sequence can be anywhere in the array)
- same but a then not-b then b
- assert that an array contents matches sequence x, y, z exactly
I think I'm struggling partly because the doc prose is more of the reference style (which I find quite abstract, though of course that's highly subjective!) rather than how-to/use-case style. I think if it contained more worked examples I'd have an easier time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contain-element
is there since 2016. This PR adds contain-elements
which is really just a subset check.
For your scenarios:
assert that an array contains particular elements/matches (without caring about ordering)
contain-element / contain-elements / ["a", "b"] (this is an alias for contain-elements) will satisfy this
assert that an array contents matches sequence x, y, z exactly
equal
assert that an array contains a sequence of particular elements/matches (e.g. contains a then any-single-element then b, but that sequence can be anywhere in the array)
same but a then not-b then b
I don't think this is supported today, sounds like a subset of matchers where order matters. I'm curious about the real-world use-case of this.. can be a new matcher that's added in 4.X
I think if it contained more worked examples I'd have an easier time.
I love this suggestion, I wonder if it should be:
- A new section in manual.md
- A new doc that's linked to from manual.md (examples.md?)
- Just an example folder(s) of yaml files. Could use either the
matching
or maybe real world examples?
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a new section of the manual.md would work well.
Would you be interested in a PR that adds https://www.mkdocs.org/ -> github pages for documentation?
Co-authored-by: Peter Mounce <[email protected]>
Co-authored-by: Peter Mounce <[email protected]>
I like the input suggestion. The problem that I've been having is striking a balance between noisy default output and useful default outputs.
And the different executions:
Errors are caught like other goss errors are. That said, I think the tansforms need more clearer error messages and to explain which transform encountered it, since this isn't very clear.
|
My preference is for an error to tell me all of the inputs and all of the outputs, so that I don't need to run the test again to find them. I think that for machine-state assertions, that's more-than-normally valuable as a default due to the risk of intermittent failures. I think that because machine-state is not an isolated system; it's fraught with external influencers, dependencies, and those squishy meatbags changing things on what would otherwise be shining clean examples of computational perfection. :)
Maybe a gitter.im or community slack? I don't have any good suggestions, sorry. |
@ripienaar Adding you here since you're pretty active with major goss changes as of late. This is probably the defining feature of v0.4.0, would love your input. The major change is the ability to do transforms and compare "15" <= 15, or use At this point, I think it's just a discussion about UI and sane defaults, but feel free to review any part of this PR. For example, using tap output and the example from the comment above #576 (comment) currently the PR defaults to a "quiet" output
This can be changed by using
@petemounce is proposing removing the include_raw and just having the transform chain include the "input" as part of it.
|
@petemounce This is very close now, I think.. The failure is in the windows test:
Any objection to this change/fix? ac0e1ae |
* Migrate from go-ps to gopsutil for better process detection (#597) * Migrate from go-ps to gopsutil for better process detection * Remove debugging code * Mount (#601) * Add stale.yml config * Change stale.yml label * Add VfsOpts to mount * Use mountinfo filter * Fix build failures * Normalize seliux mount options for integration testing * Attempt to normalize tests across docker installs and fix osx test setup * Try different osx image * Rename binaries to strip '-alpha' (#671) * Rename binaries to strip '-alpha' As discussed in #663 (comment); strip the -alpha naming, keep the messaging, keep the --use-alpha flag and env-var. This allows people to remove edge-cases from their installation, yet retains the clearly-set expectations of support being community-driven. Fixes #651. * set expectations for future-us re: macOS+Windows vs linux * fix doc * F openrc runlevels (#668) * service: Add support for OpenRC runlevels * Omit empty runlevel * Update documentation * Update integration tests * Use consistent pattern for configu default values - Remove struct tag `default` value reflection * Make runLevels a testable attribute * empty commit to try to trigger travis * Update trusty to reflect precise changes Co-authored-by: Berney <[email protected]> * POC/DRAFT: Add transforms (#576) * Add transforms * wip add transforms work * Add some examples to make sense of this * Handle floats/ints better * Convert validatecontails to gomega * wip * Add gjson and contain-substring * wip * Subclass all matchers adding String() * I don't evne know what this is.. should have checked it in back when I wrote it * Migrate GomegaMatcher -> GossMatcher * lots of changes + drop json-iterator * Move output related logic to outputs * Add include_raw flag * Add bench output format * Update have_patterns to check input. Add stubs to make Not matcher act as omegaMatcher * SetEscapeHTML(false) for semver constraint * Ensure have-Key_with_value works * remove have-key-with-value * Fix error compact output. Add oMegaMatcher stubs. Ensure matcher vs m consistent * Cleanup matchers * Cleanup TestResult struct and remove need for dyno * Update all deps * Update tranformer tests and add summary line to bench output format * Update readme * Conditionally print missing and empty values. Sanitize found and expected values for consistency * Check value is valid before checking IsNil() * Initial docs change * WIP fix for pretty print * Swap order on video vs blog for dgoss docs * Updated all tests except for semver * Fix all unit tests * Fix detection of when matcher is set * Update documentation * Update docs/manual.md Co-authored-by: Peter Mounce <[email protected]> * Update docs/manual.md Co-authored-by: Peter Mounce <[email protected]> * gjson: Validate json before doing gets * Make errors more clear * Remove unused field * Bump go version * Update doc * Fix regression with gomega errors * Fix #262 no need to add file size by default * Fix output formats * Use proper quoting for windows test * Add error checking for gjson path not found Co-authored-by: Peter Mounce <[email protected]> * Fix mountinfo splitting when there's a quoted comma * Improve have_patterns error message * Add syntax checks for type casting * Rename file.contains to file.contents * ToString converter add suppert for []interface{} * Convert headers to lowercase. fixes #760 * Fix contains -> contents, lowercase headers and contain-element float64 * Enhance all resources to support key override closes #518, closes #742 * Sort output in documentation format closes #416 * Fix typo in contain_element message * Track start and end times per-test This also changes the way total time is calculated in the output summary. total time = endTime of last test - start time of first test * Cache test results in serve instead of output closes #612 * Use exit code 78 if test file is unparseable closes #317 * Full EVR for rpm and fix failing tests * more tests * Increase test coverage * Fixed numeric eq bug * Make output more consistent * Update dependencies * Fix some merge conflicts * Revert "Migrate from go-ps to gopsutil for better process detection (#597)" This reverts commit 85ce2c8. * Revert 1fe5571 change http.headers back to io.Reader * FIX: use filepath.Join() in order to be OS agnostic * FIX: only run matcher_tests on linux * FIX: replace failing www.microsoft.com test * FIX: trusty dockerfile merge regression * Run all tests (except failing prometheus) as part of CI. Seems they were not running * Remove bench output, add some comments * Changed: include_raw by default, provide exclude_raw as a format flag * Changed: go-funk -> lo * Changed: temp fix to deal with httpbin.org slowness. Need to move to offline testing to avoid flakiness * Locking down version in install.sh, to avoid RC being installed --------- Co-authored-by: Peter Mounce <[email protected]> Co-authored-by: Berney <[email protected]> Co-authored-by: Peter Mounce <[email protected]>
This is to showcase some of the changes I have in mind for #538 to do a major enhancement on the goss/gomega language. Check out the examples folder for an idea of where I'm going with this.
This is horrible code that needs to be cleaned up, just tried to get a quick braindump out there since the code explains it a lot better than my ticket.
For usage examples take a look at this:
https://github.com/aelsabbahy/goss/tree/add_transforms/examples