V4 new #814

aelsabbahy · 2023-06-25T21:30:49Z

Checklist

make test-all (UNIX) passes. CI will also test this
unit and/or integration tests are included (if applicable)
documentation is changed or added (if applicable)

Description of change

Overview

This PR is a combination of v4 changes + a bunch of misc fixes/additions. My hope is to get this released sooner rather than later so future PRs/changes can be smaller and incremental.

The major changes with v4 is the ability to compare different types and more advanced string matching

note: Manual has been updated to document most (if not all) new matchers, also testdata/ folder has ~80% coverage of the matcher language.

For example:

command:
  echo_test:
    # command that outputs JSON, but this could be anything, http response, command output, etc
    exec: |
      echo '{"string_value": "15"}'
    exit-status: 0
    stdout:
      # advanced string parsing
      gjson:
        # extract "string_value"
        string_value:
          and:
            # The value is numerically <= 20 (auto type conversion)
            - le: 20
            # The value is numerically 15
            - 15
            # Equal does a strict check, the types have to match, hence string_value is "15" but not 15
            - equal: "15"
            - not: {equal: 15}

This conversion also allows treating an io.Reader (memory efficient line-by-line parsing) as a whole string (when compared with a string). This has the benefit of showing the full command output, which was a long-requested feature. For example:

$ cat goss.yaml
command:
  echo_test:
    exec: |
      echo 'hello world'
    exit-status: 0
    stdout: |
      goodbye world

$ goss v
.F

Failures/Skipped:

Command: echo_test: stdout:
Expected
    "hello world\n"
to equal
    "goodbye world\n"

Total Duration: 0.001s
Count: 2, Failed: 1, Skipped: 0

Changes

Added: transforms support (closes Feature Request: Improve gomega matchers #538)
- Allows comparison of string to int, ex: ensuring a systctl output below/above a specific number (closes Clarify kernel-param value as string #220)
- Allows JSON parsing/validation support (closes add plugin support for goss #578)
- Allows comparing command output to a string, this will display the failed output on failure (closes Expose actual command stdout / stderr on the goss test result #483)
- -o include_raw will show the non-transformed value
Added: Some new matchers
- equal matcher added to do strict type comparison (e.x. string to string)
- contain-substring
- have-patterns - This mimics the existing default behavior of file contents and command output
- contain-elements - checks that an array value matches a set of matchers
Added: runlevels support for service
Added: VfsOpts to mount check, (closes Check for mount options doesn't work #443)
Added: option to sort output (closes Sort output in Documentation format #416)
Added: All resources now support key override (closes DNS checks should have hostname as attribute, not key #518, closes Ability to isolate tests #742)
Added: Use exit code 78 if test file is unparseable (closes Separate exit code for test parsing errors? #317)
Changed: removed alpha from non-linux binary names
Changed: StartTime is no longer calculated from goss start. Calculated from first test start, this allows accurate reporting when showing a cached result
Changed: file.contains to file.contents
Changed: Completely re-worked matchers and shared output logic
Changed: Cache test results in serve instead of output (closes goss serve cache should separate results from output #612)
Changed: RPM Version is now full EVR (-qf "%|EPOCH?{%{EPOCH}:}:{}|%{VERSION}-%{RELEASE}), this opens the door for proper rpm version comparison in the future
Added: Significantly improved test coverage of matcher, output code
Added: bench output format
Changed: Doc to reference cat -E /proc/<PID>/comm to avoid whitespace issues (closes ps output is not trimmed before checking for running process #762)
Changed: Removed file.Size from autoadd (closes Either drop file/size from add/autoadd OR add md5 and sha to it. #262)
Added: matcher.as-reader this allows the matcher string to behave as an io.Reader output. Can be useful for testing
Removed: json_online output format, use: goss v -f json -o oneline

Decisions

Should `-o include_raw` be enabled by default, and the option becomes `-o exclude_raw`

$  cat goss.yaml
command:
  echo_test:
    exec: |
      echo '{"string_value": "15"}'
    exit-status: 0
    stdout:
      gjson:
        string_value:
          le: 5

Without include_raw:

$ goss v
.F

Failures/Skipped:

Command: echo_test: stdout:
Expected
    15
to be numerically le
    5
the transform chain was
    [{"gjson":{"Path":"string_value"}},{"to-numeric":{}}]

Total Duration: 0.001s
Count: 2, Failed: 1, Skipped: 0

With -o include_raw:

$ goss v -o include_raw
.F

Failures/Skipped:

Command: echo_test: stdout:
Expected
    15
to be numerically le
    5
the transform chain was
    [{"gjson":{"Path":"string_value"}},{"to-numeric":{}}]
the raw value was
    "{\"string_value\": \"15\"}\n"

Total Duration: 0.001s
Count: 2, Failed: 1, Skipped: 0

reverted (defer to later)

These were changes living on the v4 branch. However, they are either bad fixes, or need more polish before release

Changed: http.headers from io.reader -> array
- This is a good discussion point on whether headers should be treated like an io.Reader or array. There are pros/cons to this.. however, I'm avoiding the breaking change for now.
Changed: Convert headers to lowercase. HTTP Header X-XSS-Protection does not match #760
- Not sure how I like this solution, it breaks existing tests.. and not sure it provides too much value. Will discuss on HTTP Header X-XSS-Protection does not match #760 prior to rolling out the change
Changed: Migrate from go-ps to gopsutil for better process detection (Migrate from go-ps to gopsutil for better process detection #597)
- I was hoping this woulud fix some of the issues with go-ps.. and it does. However, it reduces the ability to discover the process name for the user. The logic for gopsutil is here. Deferring decision for now.

* Migrate from go-ps to gopsutil for better process detection * Remove debugging code

* Add stale.yml config * Change stale.yml label * Add VfsOpts to mount * Use mountinfo filter

* 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

* 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]>

* 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]>

closes #518, closes #742

closes #416

This also changes the way total time is calculated in the output summary. total time = endTime of last test - start time of first test

closes #612

closes #317

…ere not running

ripienaar

Too big to realistically give a thorough review but overall this looks really great

ripienaar · 2023-06-26T08:27:22Z

resource/command.go

-	Skip       bool     `json:"skip,omitempty" yaml:"skip,omitempty"`
+	Title      string  `json:"title,omitempty" yaml:"title,omitempty"`
+	Meta       meta    `json:"meta,omitempty" yaml:"meta,omitempty"`
+	id         string  `json:"-" yaml:"-"`


no harm, so just a comment - the json and yaml tags here not needed as the marshallers wont include private struct members

petemounce

I've reviewed all the parts that are not the matchers; 58/135. I'll review the rest in a follow-up today.

petemounce · 2023-07-05T09:31:59Z

.travis.yml

+    - release/goss-darwin-amd64
+    - release/goss-darwin-amd64.sha256


Same for windows...?

petemounce · 2023-07-05T09:32:35Z

ci/go-test.sh

@@ -3,7 +3,7 @@ set -euo pipefail

 command -v go

-go test -coverprofile="c.out" "${1}"
+go test -coverpkg=./... ./... -skip '^TestPrometheus' -coverprofile="c.out"


Mind commenting the reason for the skip?

Ooh, this one is easy.. it fails 😄

To be fair, I did run it on the main branch and it fails there too. Basically, the way that shell script was written it wasn't actually running those tests.. so they may have never worked.. not sure.. didn't try to check out the original commit to see.

I can take a look as a follow-up PR, or since you're familiar with the code, maybe give it a second look? Maybe I broke something a while back and not noticed.

Hahaha, ok. Reasonably sure it ran and passed when introduced, but I also haven't gone back and checked.

Once this is merged, I propose to spend some time enhancing our build & test pipeline.

petemounce · 2023-07-05T09:33:17Z

cmd/goss/goss.go

@@ -416,14 +415,14 @@ func addAlphaFlagIfNeeded(app *cli.App) {
 	if runtime.GOOS == "darwin" || runtime.GOOS == "windows" {
 		app.Flags = append(app.Flags, cli.StringFlag{
 			Name:   "use-alpha",
-			Usage:  fmt.Sprintf("goss is alpha-quality. Set to 1 to use anyway."),
+			Usage:  fmt.Sprintf("goss on macOS/Windows is alpha-quality. Set to 1 to use anyway."),


Contradicts expectation from binary name? https://github.com/goss-org/goss/pull/814/files#diff-6ac3f79fc25d95cd1e3d51da53a4b21b939437392578a35ae8cd6d5366ca5485R44-R45

I guess at this point I can remove the alpha flag altogether and just document that it's not as featureful/stable as Linux..

Looking back at the history of this, I'll put it back on your plate.. What are your thoughts on alpha vs non-alpha.

Your PR renamed the files.. seems they're fairly stable from your comment 2-3 years back.

#671

I'd be fine with renaming each binary to remove the alpha segment. We still have the flag/env-var & message, and we have integration tests that run and pass for a lot of the functionality; missing or broken things can be iterated incrementally at need.

petemounce · 2023-07-05T09:38:10Z

matcher_test.go

+)
+
+var (
+	update = flag.Bool("update", false, "update the golden files of this test")


Mind adding a comment for how and when to use this?

Added comment

petemounce · 2023-07-05T09:38:48Z

matcher_test.go

+	for _, outFile := range files {
+		outFile := outFile
+		parts := strings.Split(outFile, ".")
+		tfName := fmt.Sprintf("%s.yaml", strings.TrimPrefix(parts[0], "testdata/out_"))


What's tf in this context? Please expand or comment?

"test file" renamed to specFile

petemounce · 2023-07-05T09:41:03Z

serve_test.go

@@ -268,14 +268,14 @@ func TestServeCacheNegotiatingContent(t *testing.T) {
 		logOutput.Reset()
 	})

-	t.Run("immediately re-request but different accept header, cache should be cold", func(t *testing.T) {
+	t.Run("immediately re-request but different accept header, cache should be warm", func(t *testing.T) {


🤕 on my part!

Nah, those tests were great for implementing this feature. Made my life so much easier.

petemounce

LGTM

matchers/be_numerically_matcher.go

petemounce · 2023-07-05T19:32:57Z

outputs/prometheus.go

+			if startTime.IsZero() || tr.StartTime.Before(startTime) {
+				startTime = tr.StartTime
+			}


Under what circumstances is this needed...?

It's a loop finding the earliest start time. Before, startTime was calculated at program start, and endTime was when the output was printing. Now, it's calculated from the start time of the first test and endtime of the last test.

The reason for this is when caching testResults and rendering them in different formats, the start/end time needs to be stable. Without this, different output formats would show different times depending on when they were run.

If that's not present as a comment someplace, please could it become present? I don't think I would have inferred that without the explanation.

petemounce · 2023-07-05T19:34:32Z

outputs/outputs.go

+}
+
+func prettyPrint(i interface{}, indent bool) string {
+	// JSON doesn't like non-string keys


It's sooo nitpicky.

petemounce · 2023-07-06T17:12:59Z

outputs/bench.go

+	"github.com/goss-org/goss/util"
+)
+
+type Bench struct{}


Seems like a new output; mention in docs?

Mentioned in docs, also mentioned it may not be stable in future releases.. Was using it for debugging and got tired of it not being part of goss =)

Logging may replace it, and it may get deleted in the future.

Thinking through it, part of me wonders if maybe I should remove it from this PR. Leaning towards that, but will sleep on it.

🤷 . I'd say leave it in if it was useful.

Slept on it, removing it for now. Initial logging support was added as part of PR a few months back. I have some thoughts on expanding logging to be a lot more useful. Once that story concludes, perhaps the bench output will not be useful.

I'll open up some issues after this PR is merged.

petemounce · 2023-07-06T17:15:30Z

resource/file.go

-	if len(f.Contains) > 0 {
-		results = append(results, ValidateContains(f, "contains", f.Contains, sysFile.Contains, skip))
+	if isSet(f.Contains) {
+		fmt.Fprintf(os.Stderr, "DEPRECATION WARNING: file.contains has been renamed to file.contents\n")


Mention these within the manual somewhere/how? Or maybe make an upgrade-notes for v4?

I'll add it to the upgrade notes

petemounce · 2023-07-06T17:22:34Z

Should -o include_raw be enabled by default, and the option becomes -o exclude_raw

I would prefer -o include_raw to be the default, because I think that's a higher UX. I would be frustrated if I got a failure, had to re-run with -o include_raw and the 2nd run passed; indicating the test is flaky somehow. I think that it's best to give the data up-front, because in the context of machine-assertions (and especially when some can be remote-assertions like http, addr) the chance of flakes is higher than in "normal" test frameworks.

petemounce · 2023-07-06T17:23:50Z

(Also - this is great work; thank you!)

aelsabbahy · 2023-07-06T17:59:49Z

Awesome, thanks for the thorough feedback. I'll address it over the coming week and hopefully this gets merged/released after that.

I would prefer -o include_raw to be the default

This was my leaning as well, for all the reasons you mentioned.

aelsabbahy

Added some comments, will address more over the week.

aelsabbahy · 2023-07-11T21:21:02Z

matcher_test.go

+	for _, outFile := range files {
+		outFile := outFile
+		parts := strings.Split(outFile, ".")
+		tfName := fmt.Sprintf("%s.yaml", strings.TrimPrefix(parts[0], "testdata/out_"))


"test file" renamed to specFile

matchers/be_numerically_matcher.go

aelsabbahy · 2023-07-11T21:24:34Z

outputs/bench.go

+	"github.com/goss-org/goss/util"
+)
+
+type Bench struct{}


Mentioned in docs, also mentioned it may not be stable in future releases.. Was using it for debugging and got tired of it not being part of goss =)

Logging may replace it, and it may get deleted in the future.

Thinking through it, part of me wonders if maybe I should remove it from this PR. Leaning towards that, but will sleep on it.

aelsabbahy · 2023-07-11T21:30:51Z

outputs/prometheus.go

+			if startTime.IsZero() || tr.StartTime.Before(startTime) {
+				startTime = tr.StartTime
+			}


It's a loop finding the earliest start time. Before, startTime was calculated at program start, and endTime was when the output was printing. Now, it's calculated from the start time of the first test and endtime of the last test.

The reason for this is when caching testResults and rendering them in different formats, the start/end time needs to be stable. Without this, different output formats would show different times depending on when they were run.

aelsabbahy · 2023-07-11T21:31:42Z

serve_test.go

@@ -268,14 +268,14 @@ func TestServeCacheNegotiatingContent(t *testing.T) {
 		logOutput.Reset()
 	})

-	t.Run("immediately re-request but different accept header, cache should be cold", func(t *testing.T) {
+	t.Run("immediately re-request but different accept header, cache should be warm", func(t *testing.T) {


Nah, those tests were great for implementing this feature. Made my life so much easier.

Need to move to offline testing to avoid flakiness

aelsabbahy · 2023-07-17T17:32:11Z

Removed bench output
Temp solution to try to improve test flakiness

Addressed some (most/all?) of the feedback here.

@petemounce let me know if all looks good, if I don't hear back in a week, I'll merge it.

aelsabbahy · 2023-07-19T01:16:01Z

Modified install.sh (locked down version) to avoid breaking anyone. Merging, will create a release candidate release.

aelsabbahy and others added 30 commits September 7, 2020 10:10

Migrate from go-ps to gopsutil for better process detection (#597)

85ce2c8

* Migrate from go-ps to gopsutil for better process detection * Remove debugging code

Mount (#601)

6c123f8

* Add stale.yml config * Change stale.yml label * Add VfsOpts to mount * Use mountinfo filter

Merge master into v4

bb9c5a6

Fix build failures

3f8f586

Normalize seliux mount options for integration testing

142c562

Attempt to normalize tests across docker installs and fix osx test setup

e787e01

Try different osx image

28634ee

Fix mountinfo splitting when there's a quoted comma

a6d2f2a

Improve have_patterns error message

6bf1c78

Add syntax checks for type casting

4be04f9

Rename file.contains to file.contents

dfb1186

ToString converter add suppert for []interface{}

12cef65

Convert headers to lowercase. fixes #760

1fe5571

Fix contains -> contents, lowercase headers and contain-element float64

2ebb1f8

Enhance all resources to support key override

bf22f82

closes #518, closes #742

Sort output in documentation format

61922e6

closes #416

Fix typo in contain_element message

14c3d45

Track start and end times per-test

db012e4

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

04eea68

closes #612

Use exit code 78 if test file is unparseable

2a8c9e5

closes #317

Full EVR for rpm and fix failing tests

ef62d85

more tests

aa990cf

Increase test coverage

5baad14

Fixed numeric eq bug

a83de2b

Make output more consistent

07f40df

Update dependencies

46f455a

Merge remote-tracking branch 'origin/master' into v4

e2e0037

aelsabbahy added 6 commits June 24, 2023 20:27

Revert 1fe5571 change http.headers back to io.Reader

28c1e53

FIX: use filepath.Join() in order to be OS agnostic

b50a296

FIX: only run matcher_tests on linux

9e7b3db

FIX: replace failing www.microsoft.com test

ef7c933

FIX: trusty dockerfile merge regression

d8c9c93

Run all tests (except failing prometheus) as part of CI. Seems they w…

2a48c91

…ere not running

aelsabbahy requested a review from ripienaar June 25, 2023 21:33

ripienaar previously approved these changes Jun 26, 2023

View reviewed changes

aelsabbahy mentioned this pull request Jun 26, 2023

The future of Goss #724

Closed

petemounce reviewed Jul 5, 2023

View reviewed changes

petemounce previously approved these changes Jul 6, 2023

View reviewed changes

aelsabbahy commented Jul 11, 2023

View reviewed changes

aelsabbahy added 4 commits July 15, 2023 09:53

Remove bench output, add some comments

e7e7841

Changed: include_raw by default, provide exclude_raw as a format flag

a77d2bb

Changed: go-funk -> lo

a41dac2

Changed: temp fix to deal with httpbin.org slowness.

d178d97

Need to move to offline testing to avoid flakiness

aelsabbahy dismissed stale reviews from petemounce and ripienaar via d178d97 July 17, 2023 17:29

aelsabbahy mentioned this pull request Jul 17, 2023

goss add command to set default assertions for empty stdout/stderr #820

Closed

petemounce previously approved these changes Jul 17, 2023

View reviewed changes

Locking down version in install.sh, to avoid RC being installed

783a2b4

aelsabbahy dismissed petemounce’s stale review via 783a2b4 July 18, 2023 16:43

aelsabbahy merged commit 2e5720b into master Jul 19, 2023

aelsabbahy deleted the v4_new branch July 19, 2023 01:16

samrocketman mentioned this pull request Mar 26, 2024

goss validate command does not print command output #890

Closed

		- release/goss-darwin-amd64
		- release/goss-darwin-amd64.sha256

V4 new #814

V4 new #814

Conversation

aelsabbahy commented Jun 25, 2023 • edited Loading

Checklist

Description of change

Overview

The major changes with v4 is the ability to compare different types and more advanced string matching

Changes

Decisions

Should -o include_raw be enabled by default, and the option becomes -o exclude_raw

reverted (defer to later)

ripienaar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemounce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemounce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemounce commented Jul 6, 2023

petemounce commented Jul 6, 2023

aelsabbahy commented Jul 6, 2023 • edited Loading

aelsabbahy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aelsabbahy commented Jul 17, 2023

aelsabbahy commented Jul 19, 2023

aelsabbahy commented Jun 25, 2023 •

edited

Loading

Should `-o include_raw` be enabled by default, and the option becomes `-o exclude_raw`

aelsabbahy commented Jul 6, 2023 •

edited

Loading