-
-
Notifications
You must be signed in to change notification settings - Fork 347
test, integration: Add an integration test framework #368
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,263 @@ | ||
| // Mgmt | ||
| // Copyright (C) 2013-2018+ James Shubin and the project contributors | ||
| // Written by James Shubin <james@shubin.ca> and the project contributors | ||
| // | ||
| // This program is free software: you can redistribute it and/or modify | ||
| // it under the terms of the GNU General Public License as published by | ||
| // the Free Software Foundation, either version 3 of the License, or | ||
| // (at your option) any later version. | ||
| // | ||
| // This program is distributed in the hope that it will be useful, | ||
| // but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| // GNU General Public License for more details. | ||
| // | ||
| // You should have received a copy of the GNU General Public License | ||
| // along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| package integration | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "io/ioutil" | ||
| "os" | ||
| "path" | ||
| "sort" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestInstance0(t *testing.T) { | ||
| code := ` | ||
| $root = getenv("MGMT_TEST_ROOT") | ||
|
|
||
| file "${root}/mgmt-hello-world" { | ||
| content => "hello world from @purpleidea\n", | ||
| state => "exists", | ||
| } | ||
| ` | ||
| m := Instance{ | ||
| Hostname: "h1", // arbitrary | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would just drop these two default parameters, like you mention the hostname is arbitrary, why not initialise them in the generic pattern or instance? Also is there any reason to preserve the result if the test passes or to not save the result if the test fails? Tests should be setup to run in (os level) temporary directories anyways, where garbage collection is common, no need to add high level management of temporary files to the suite itself imho. Unless they really can create enormous amounts of data quickly (not like this case a few directories and kb of files). |
||
| Preserve: true, | ||
| } | ||
| if err := m.SimpleDeployLang(code); err != nil { | ||
| t.Errorf("failed with: %+v", err) | ||
| if output, err := m.CombinedOutput(); err == nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seeing how this was added in a later commit it saddens me that you are repeating mistakes that where already avoided in my implementation. So now we have both wasted valuable time on the same thing.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ?? Not sure I understand the point, but please don't be sad. It's just code. This isn't a perfect process either.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, it's just code. I'm just sad that the experience and lessons that are in my PR where lost when you rewrote your implementation only to add them again later on when you've spend time discovering it yourself. |
||
| t.Errorf("logs from instance:\n\n%s", output) | ||
| } | ||
| return | ||
| } | ||
| d := m.Dir() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Within a testcase there is really no benefit to having 9 lines of code to just verify a file exists (you're not even verifying the content yet, it could be empty, testcase would false positive). All it does is make the testcase harder to interpret and increase the cognitive load. This can just be a simple single line assertion and it's a common enough one that it can be added to patterns or the instance. |
||
| t.Logf("test ran in:\n%s", d) | ||
| root := path.Join(d, RootDirectory) | ||
| file := path.Join(root, "mgmt-hello-world") | ||
| _, err := os.Stat(file) | ||
| if err != nil { | ||
| t.Errorf("could not find file: %+v", err) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| func TestInstance1(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given this is an table driven test it is expected that a certain 'class' of tests are run in this testcase. TestInstance1 does not describe this very well. A better name would be something like: |
||
| type test struct { // an individual test | ||
| name string | ||
| code string // mcl code | ||
| fail bool | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please document the intention of this boolean, it saved having to dive into the code logic, something like: |
||
| expect map[string]string | ||
| } | ||
| values := []test{} | ||
|
|
||
| { | ||
| code := Code(` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a similar approach that I had initially envisioned. I didn't spend time yet constructing this as I first wanted to know which common patterns emerge. Some brainstorming I did on this topic is #353 (comment) Where the environment setup (instances/cluster) and validations are generalised so they can be reused across tests. |
||
| $root = getenv("MGMT_TEST_ROOT") | ||
|
|
||
| file "${root}/mgmt-hello-world" { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm glad you kept this in.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course! Your idea of using |
||
| content => "hello world from @purpleidea\n", | ||
| state => "exists", | ||
| } | ||
| `) | ||
| values = append(values, test{ | ||
| name: "hello world", | ||
| code: code, | ||
| fail: false, | ||
| expect: map[string]string{ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. expectFilenameWithContent is a better description adding some context about the key and value of this map.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, this is temporary. I agree that |
||
| "mgmt-hello-world": "hello world from @purpleidea\n", | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| for index, test := range values { // run all the tests | ||
| t.Run(fmt.Sprintf("test #%d (%s)", index, test.name), func(t *testing.T) { | ||
| code, fail, expect := test.code, test.fail, test.expect | ||
|
|
||
| m := Instance{ | ||
| Hostname: "h1", | ||
| Preserve: true, | ||
| } | ||
| err := m.SimpleDeployLang(code) | ||
| d := m.Dir() | ||
| if d != "" { | ||
| t.Logf("test ran in:\n%s", d) | ||
| } | ||
|
|
||
| if !fail && err != nil { | ||
| t.Errorf("failed with: %+v", err) | ||
| if output, err := m.CombinedOutput(); err == nil { | ||
| t.Errorf("logs from instance:\n\n%s", output) | ||
| } | ||
| return | ||
| } | ||
| if fail && err == nil { | ||
| t.Errorf("passed, expected fail") | ||
| return | ||
| } | ||
|
|
||
| if expect == nil { // test done early | ||
| return | ||
| } | ||
|
|
||
| files := []string{} | ||
| for x := range expect { | ||
| files = append(files, x) | ||
| } | ||
| sort.Strings(files) // loop in a deterministic order | ||
| for _, f := range files { | ||
| filename := path.Join(d, RootDirectory, f) | ||
| b, err := ioutil.ReadFile(filename) | ||
| if err != nil { | ||
| t.Errorf("could not read file: `%s`", filename) | ||
| continue | ||
| } | ||
| if expect[f] != string(b) { | ||
| t.Errorf("file: `%s` did not match expected", f) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestCluster0(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing docs |
||
| // TODO: implement a simple test for documentation purposes | ||
| } | ||
|
|
||
| func TestCluster1(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing docs |
||
| type test struct { // an individual test | ||
| name string | ||
| code string // mcl code | ||
| fail bool | ||
| hosts []string | ||
| expect map[string]map[string]string // hostname, file, contents | ||
| } | ||
| values := []test{} | ||
|
|
||
| { | ||
| code := Code(` | ||
| $root = getenv("MGMT_TEST_ROOT") | ||
|
|
||
| file "${root}/mgmt-hostname" { | ||
| content => "i am ${hostname()}\n", | ||
| state => "exists", | ||
| } | ||
| `) | ||
| values = append(values, test{ | ||
| name: "simple pair", | ||
| code: code, | ||
| fail: false, | ||
| hosts: []string{"h1", "h2"}, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explicit hostnames makes more sense here compared to setting the hostname for the single instance test. |
||
| expect: map[string]map[string]string{ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So when there is a test case where
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed! See reply in #368 (comment) |
||
| "h1": { | ||
| "mgmt-hostname": "i am h1\n", | ||
| }, | ||
| "h2": { | ||
| "mgmt-hostname": "i am h2\n", | ||
| }, | ||
| }, | ||
| }) | ||
| } | ||
| { | ||
| code := Code(` | ||
| $root = getenv("MGMT_TEST_ROOT") | ||
|
|
||
| file "${root}/mgmt-hostname" { | ||
| content => "i am ${hostname()}\n", | ||
| state => "exists", | ||
| } | ||
| `) | ||
| values = append(values, test{ | ||
| name: "hello world", | ||
| code: code, | ||
| fail: false, | ||
| hosts: []string{"h1", "h2", "h3"}, | ||
| expect: map[string]map[string]string{ | ||
| "h1": { | ||
| "mgmt-hostname": "i am h1\n", | ||
| }, | ||
| "h2": { | ||
| "mgmt-hostname": "i am h2\n", | ||
| }, | ||
| "h3": { | ||
| "mgmt-hostname": "i am h3\n", | ||
| }, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| for index, test := range values { // run all the tests | ||
| t.Run(fmt.Sprintf("test #%d (%s)", index, test.name), func(t *testing.T) { | ||
| code, fail, hosts, expect := test.code, test.fail, test.hosts, test.expect | ||
|
|
||
| c := Cluster{ | ||
| Hostnames: hosts, | ||
| Preserve: true, | ||
| } | ||
| err := c.SimpleDeployLang(code) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking that the instance and cluster types could share a common interface. This way generic test patterns like these can be shared between the two easily. As a single instance is just a cluster of 1 when you think about it. So if the interface is similar you just loop over 1 instance in the cluster to init, get the results, etc.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was actually thinking about this, but couldn't find a benefit to actually doing it. At least not until there's some more general thing that wants to consume that interface. However, in that case, we'd probably just only pass in "clusters", which might have one instance or might not. |
||
| if d := c.Dir(); d != "" { | ||
| t.Logf("test ran in:\n%s", d) | ||
| } | ||
| instances := c.Instances() | ||
|
|
||
| if !fail && err != nil { | ||
| t.Errorf("failed with: %+v", err) | ||
| for _, h := range hosts { | ||
| if output, err := instances[h].CombinedOutput(); err == nil { | ||
| t.Errorf("logs from instance `%s`:\n\n%s", h, output) | ||
| } | ||
| } | ||
| return | ||
| } | ||
| if fail && err == nil { | ||
| t.Errorf("passed, expected fail") | ||
| return | ||
| } | ||
|
|
||
| if expect == nil { // test done early | ||
| return | ||
| } | ||
|
|
||
| for _, h := range hosts { | ||
| instance := instances[h] | ||
| d := instance.Dir() | ||
| hexpect, exists := expect[h] | ||
| if !exists { | ||
| continue | ||
| } | ||
|
|
||
| files := []string{} | ||
| for x := range hexpect { | ||
| files = append(files, x) | ||
| } | ||
| sort.Strings(files) // loop in a deterministic order | ||
| for _, f := range files { | ||
| filename := path.Join(d, RootDirectory, f) | ||
| b, err := ioutil.ReadFile(filename) | ||
| if err != nil { | ||
| t.Errorf("could not read file: `%s`", filename) | ||
| continue | ||
| } | ||
| if hexpect[f] != string(b) { | ||
| t.Errorf("file: `%s` did not match expected", f) | ||
| } | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
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.
missing docs
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.
Docs would be nice. Missing docs in tests isn't my no.1 prio at this point though. Sorry!
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.
Imho, tests are important, more important than code, as that is just the implementation, the test is the proof that code provides the functionality that is required. So for me tests, their implementation and documentation are equal if not higher priority.