tests: expand coreos-install test options #250

arithx · 2017-09-12T00:34:43Z

Also adds support for local image file caching. The UseLocalServer flag can be used to pull from the local HTTP server.

Adds flags to the Test object for the following options:

Disk Size
- Size of the disk the image is being installed onto. Defaults to 10Gb
Board
LocalImage
Version
BaseURL
Channel
UseLocalServer

The following tests have switched to pulling from the local HTTP server:

TestCoreosInstall/Ignition_Test
TestCoreosInstall/CloudConfig_Test
TestCoreosInstall/Disk_Size_too_small_-_Remote

The following tests are added in this PR:

TestCoreosInstall/Disk_Size_too_small_-_Local
TestCoreosInstall/Disk_Size_too_small_-_Remote
TestCoreosInstall/Alpha_1520.0
TestCoreosInstall/Channel_Only
TestCoreosInstall/arm64-usr_alpha_1367.5.0
TestCoreosInstall/Version_Only

bgilbert · 2017-09-12T20:37:06Z

tests/negative/disk_size.go

+ diskFile, loopDevice := test.CreateDevice(t)
+ defer test.CleanupDisk(t, diskFile, loopDevice)
+
+ opts := []string{


It'd be better to abstract out the command-line building used by the positive tests, rather than reimplementing here.

bgilbert · 2017-09-12T20:38:07Z

tests/negative/disk_size.go

+ if err == nil {
+ t.Fatalf("install passed when it shouldn't have")
+ }
+}


Should check that the partition table is wiped afterward.

bgilbert · 2017-09-12T20:38:27Z

tests/negative/disk_size.go

+ opts = append(opts, "-f", test.LocalImagePath)
+ }
+
+ err := util.Run(t, test.BinaryPath, opts...)


Should there be a test-specific regex to check for a reasonable error message?

bgilbert · 2017-09-12T20:51:41Z

tests/util/util.go

+
+ _, err = io.Copy(tmpFile, resp.Body)
+ if err != nil {
+ defer os.Remove(tmpFile.Name())


defer not needed here.

bgilbert · 2017-09-12T20:52:37Z

tests/util/util.go

+ newPath := fmt.Sprintf("%s.bin.bz2", oldPath)
+ err = os.Rename(oldPath, newPath)
+ if err != nil {
+ return ""


Delete file.

bgilbert · 2017-09-12T20:53:21Z

tests/util/util.go

+
+ resp, err := http.Get("https://stable.release.core-os.net/amd64-usr/current/coreos_production_image.bin.bz2")
+ if err != nil {
+ return ""


Delete tmpFile, or else run Get before creating the file.

bgilbert · 2017-09-12T20:58:16Z

tests/register/register.go

@@ -110,6 +115,27 @@ func (test Test) UnmountPartitions(t *testing.T, loopDevice string) {
 func (test Test) RunCoreOSInstall(t *testing.T, loopDevice string, opts ...string) {
 opts = append(opts, "-d", loopDevice)

+ // use local image if it's available and we aren't testing any version picks directly


This will mean that we're testing -f most of the time, which is the opposite of what users will do, and won't exercise the fetching and signature checking code as much as we could. I'd rather have explicit tests for -f, and default to overriding -b instead (and serving HTTP locally).

bgilbert · 2017-09-12T20:59:46Z

tests/register/register.go

@@ -120,6 +146,8 @@ func (test Test) RunCoreOSInstall(t *testing.T, loopDevice string, opts ...strin
 opts = append(opts, "-c", cloudinitPath)
 }

+ t.Logf("Running coreos-install via the following command: %s %s", test.BinaryPath, strings.Join(opts, " "))


Please trim this message down. The command line is the important part, and the extra words will make it harder to skim the logs.

arithx · 2017-09-13T00:20:04Z

Update pushed

bgilbert

Looks good, just some minor issues.

The review changes were all bunched into the last commit. If it's too hard to separate them out at this point, I'd be okay with squashing all three commits together, since this is still early code.

bgilbert · 2017-09-13T02:32:33Z

tests/negative/disk_size.go

+ Name: "Disk Size too small - Remote",
+ Func: installShouldFail,
+ DiskSize: 2 * 1024 * 1024 * 1024,
+ BaseURL: util.StringToPtr("https://stable.release.core-os.net/amd64-usr"),


Why the override? It seems as though the local file, served via HTTP, should be sufficient.

bgilbert · 2017-09-13T02:36:11Z

tests/register/register.go

 opts = append(opts, "-d", loopDevice)

+ // use local http server if we aren't testing specific flags
+ if test.Version == nil && test.BaseURL == nil && test.Channel == nil && test.Board == nil && !test.UseLocalFile {


This makes it impossible to use a completely default command line (with only -d specified). It seems as though we should have one test, at least, that does that. Maybe add e.g. a NoLocalServer flag?

bgilbert · 2017-09-13T02:39:03Z

tests/register/register.go

+ opts = append(opts, "-b", "https://127.0.0.1:8080/")
+ }
+
+ if test.UseLocalFile && test.LocalImagePath != "" {


If the test has asked to force a local file, but we don't have a path for one, we should fail rather than silently alter the command line.

bgilbert · 2017-09-13T02:41:18Z

tests/register/register.go

@@ -183,11 +238,31 @@ func (test Test) ValidateCloudConfig(t *testing.T, rootDir, config string) {
 }

 func (test Test) ValidateOSRelease(t *testing.T, rootDir string) {
- if _, err := os.Stat(filepath.Join(rootDir, "usr", "lib", "os-release")); os.IsNotExist(err) {
+ data, err := ioutil.ReadFile(filepath.Join(rootDir, "usr", "lib", "os-release"))
+ if os.IsNotExist(err) {
 t.Fatalf("/usr/lib/os-release was not found")
 } else if err != nil {
 t.Fatalf("running stat on /usr/lib/os-release: %v", err)


update message

bgilbert · 2017-09-13T02:45:44Z

tests/util/util.go

+func DownloadFile(tmpDir, name string) error {
+ file, err := os.Create(filepath.Join(tmpDir, name))
+ if err != nil {
+ return fmt.Errorf("failed to create file")


Include err in all these calls.

bgilbert · 2017-09-13T02:47:27Z

tests/util/util.go

+ http.HandleFunc(fmt.Sprintf("/%s/coreos_production_image.bin.bz2", version), server.Image)
+ http.HandleFunc(fmt.Sprintf("/%s/coreos_production_image.bin.bz2.sig", version), server.Signature)
+
+ s := &http.Server{Addr: ":8080"}


You'll probably want to listen on localhost only. Also, in general, hardcoded ports should be avoided for this sort of thing, since they prevent two copies of the test suite from running at once. You can have the OS pick an unused port and pass it through to the tests.

(I just realized the Ignition tests also hardcode the server port...)

arithx · 2017-09-13T04:13:45Z

When I make the next round of changes I'll pull them back out into the proper commits and update the commit messages to reflect their new state.

arithx · 2017-09-13T06:15:00Z

Addressed comments and split the files back out into proper commits (and updated the commit messages).

bgilbert · 2017-09-13T20:21:31Z

tests/util/util.go

+ if err != nil {
+ panic(err)
+ }
+


Return err.

bgilbert · 2017-09-13T20:23:16Z

tests/negative/disk_size.go

+ Func: installShouldFail,
+ DiskSize: 2 * 1024 * 1024 * 1024,
+ OutputRegexp: diskSize,
+ })


Can fetch from local server.

Downloads the current stable release image and drops it into a temp dir in /var/tmp or the TMPDIR env var for the overall test run. Runs a webserver which serves the version.txt, image, and image signature. Adds the following variables to the Test object: - LocalImagePath - LocalAddress - Version - BaseURL - Channel - Board - UseLocalFile - UseLocalServer

Adds an initial set of tests targeting the Version, Channel, & Board functionality.

arithx · 2017-09-13T20:40:08Z

Updated & pushed, good to merge from my end

arithx force-pushed the expanded_options branch from 0fb1b3b to e82a902 Compare September 12, 2017 18:35

bgilbert suggested changes Sep 12, 2017

View reviewed changes

arithx force-pushed the expanded_options branch from e82a902 to 44eef55 Compare September 13, 2017 00:19

bgilbert reviewed Sep 13, 2017

View reviewed changes

arithx force-pushed the expanded_options branch 2 times, most recently from b9c984d to 0713486 Compare September 13, 2017 06:13

arithx force-pushed the expanded_options branch 2 times, most recently from 0838675 to 7f22f47 Compare September 13, 2017 18:02

bgilbert approved these changes Sep 13, 2017

View reviewed changes

arithx added 3 commits September 13, 2017 13:39

tests: add version, channel, board related tests

4965691

Adds an initial set of tests targeting the Version, Channel, & Board functionality.

tests: add negative testing around disk size

de333b5

arithx force-pushed the expanded_options branch from 7f22f47 to de333b5 Compare September 13, 2017 20:39

bgilbert merged commit 6470951 into coreos:master Sep 13, 2017

arithx deleted the expanded_options branch September 13, 2017 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: expand coreos-install test options #250

tests: expand coreos-install test options #250

arithx commented Sep 12, 2017 •

edited

Loading

bgilbert Sep 12, 2017

bgilbert Sep 12, 2017

bgilbert Sep 12, 2017

bgilbert Sep 12, 2017

bgilbert Sep 12, 2017

bgilbert Sep 12, 2017

bgilbert Sep 12, 2017

bgilbert Sep 12, 2017

arithx commented Sep 13, 2017

bgilbert left a comment

bgilbert Sep 13, 2017

bgilbert Sep 13, 2017

bgilbert Sep 13, 2017

bgilbert Sep 13, 2017

bgilbert Sep 13, 2017

bgilbert Sep 13, 2017

arithx commented Sep 13, 2017

arithx commented Sep 13, 2017

bgilbert Sep 13, 2017

bgilbert Sep 13, 2017

arithx commented Sep 13, 2017

tests: expand coreos-install test options #250

tests: expand coreos-install test options #250

Conversation

arithx commented Sep 12, 2017 • edited Loading

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

arithx commented Sep 13, 2017

bgilbert 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

arithx commented Sep 13, 2017

arithx commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arithx commented Sep 13, 2017

arithx commented Sep 12, 2017 •

edited

Loading