swupd: improve test infrastructure and import tests from swupd-server #136

cmarcelo · 2018-02-02T23:27:36Z

Add testSwupd, in the same spirit of testFileSystem, to make tests less noisy.

Convert two tests to use it, to give some context for reviewing it.

Also import new tests from swupd-server.

Related to #138.

cmarcelo · 2018-02-03T05:24:55Z

Imported another test.

matthewrsj · 2018-02-04T01:22:37Z

swupd/helpers_test.go

+func newTestSwupd(t *testing.T, prefix string) *testSwupd {
+	fs := newTestFileSystem(t, prefix)
+	defer func() {
+		if t.Failed() {


Do you mean if !t.Failed()? This looks like it is cleaning up only if the test failed.

The code is correct, but due to similarity how defer cleanup is used elsewhere might be confusing. If when creating a testSwupd we have a failure, cleanup any existing directory. Otherwise let it live because the caller will call cleanup themselves.

I'll add a comment there.

Note that currently fs.cleanup won't really cleanup (because it was a failure) but will still show the helpful message with the path of the temporary directory.

Nice, thanks.

matthewrsj · 2018-02-04T01:25:40Z

commit 1: one comment
commit 2: +1

matthewrsj · 2018-02-04T01:28:04Z

commit 3: did you see https://github.com/clearlinux/mixer-tools/blob/master/swupd/create_manifests_test.go#L231 ?

matthewrsj · 2018-02-04T01:33:08Z

swupd/packs_test.go

+		}
+	}
+
+	mustHaveDir("delta/")


These rely on order right? If I'm right, can you put a comment in?

Done. Also mentioned I think we can relax this in the future. For now I think the validation function is fine as is.

matthewrsj · 2018-02-04T01:33:30Z

swupd/packs_test.go

@@ -385,8 +409,12 @@ func mustValidateZeroPack(t *testing.T, manifestPath, packPath string) {
 		if err != nil {
 			t.Fatalf("error reading pack: %s", err)
 		}
-		if hdr.Name == "staged/" || !strings.HasPrefix(hdr.Name, "staged/") {
-			continue
+		if hdr.Name == "staged/" || hdr.Name == "delta/" {


Another good place to put the comment that you've already encountered these.

The error message talks about "multiple %s found", so I think we are covered with that plus the other two comments added.

matthewrsj · 2018-02-04T01:34:14Z

swupd/packs_test.go

+			t.Fatalf("multiple entries of %s directory", hdr.Name)
+		}
+
+		if !strings.HasPrefix(hdr.Name, "staged/") {


And a comment that delta/ shouldn't have anything in it for a zero pack?

matthewrsj · 2018-02-04T01:35:14Z

commit 4: great full run test, couple comment requests.

matthewrsj · 2018-02-04T01:37:20Z

commit 5: that test wasn't really testing much of anything before. Oversight. Good update. +1

matthewrsj · 2018-02-04T01:39:47Z

swupd/create_manifests_test.go

+	defer ts.cleanup()
+
+	ts.Bundles = []string{"os-core", "test-bundle"}
+	ts.Format = 3


You don't seem to change this value again. Why 3? I recall it was 3 in the old server because Clear Linux was on format 3 when the tests were originally written, but I don't think it is relevant here.

Yes, I was just keeping the test doing as much as possible the same as before -- I guessed it was an historical reason or something. Having some tests using different formats is a win, but I think we have those around already.

I'll remove format 3 from tests that don't really depend on that.

matthewrsj · 2018-02-04T01:45:41Z

swupd/create_manifests_test.go

+
+	// TODO: Check what is the right behavior and fix the test if needed.
+	if false {
+		fileDeletedInManifest(ts.parseManifest(30, "os-core"), 20, "/foo")


os-core should definitely have the deleted file still. This will fail of course because deletes are not being persisted properly (see #134, it includes a persistDeletes test)

matthewrsj · 2018-02-04T01:46:13Z

swupd/create_manifests_test.go

+
+	// TODO: Check what is the right behavior and fix the test if needed.
+	if false {
+		fileDeletedInManifest(ts.parseManifest(40, "os-core"), 20, "/foo")


This should not fail, but will in its current state for the reason listed above.

matthewrsj · 2018-02-04T01:47:04Z

commit 6: please remove the if false { blockers. Your assumptions are correct and these tests should be failing (#134).

cmarcelo · 2018-02-05T20:02:44Z

commit 3: did you see https://github.com/clearlinux/mixer-tools/blob/master/swupd/create_manifests_test.go#L231 ?

I've missed that one, otherwise I'd written the extra tests in there, sorry about that.

@matthewrsj If you like the direction test helpers in this PR took, I'll amend the commit to remove the existing version of the test. Otherwise I'll just remove this commit and later post a PR adding the extra checks to that test.

matthewrsj · 2018-02-05T20:06:37Z

If you like the direction test helpers in this PR took, I'll amend the commit to remove the existing version of the test. Otherwise I'll just remove this commit and later post a PR adding the extra checks to that test.

Can you actually just replace the old test as part of this commit or do you think that is out-of-scope? The new test uses the new helpers, which I like.

matthewrsj · 2018-02-05T20:07:17Z

/me realizes that's what you suggested doing @cmarcelo, +1 please do.

cmarcelo · 2018-02-05T20:09:22Z

commit 6: please remove the if false { blockers. Your assumptions are correct and these tests should be failing (#134).

Problem of having tests failing is make the test suite less useful, I think the TODOs and skipping the test is fine in this case, since this PR is not pushing the fix for the issue at the same time. When the fix land, it can get rid of TODOs and skips.

cmarcelo · 2018-02-05T20:10:42Z

Your assumptions are correct and these tests should be failing (#134).

I'll update the TODOs to reflect that knowledge.

matthewrsj · 2018-02-05T20:11:12Z

Problem of having tests failing is make the test suite less useful, I think the TODOs and skipping the test is fine in this case, since this PR is not pushing the fix for the issue at the same time. When the fix land, it can get rid of TODOs and skips.

I disagree. The whole point of tests is so that they fail. It is easy to forget about TODOs and skips once something is merged. If that means we merge failing tests now at least we know what is failing and what needs to be fixed. I see no usefulness in tests that should be failing but are passing instead.

EDIT: It may not be pretty to see CI failing, but at least we know there is an issue. If that means we don't merge the tests until the fix lands, that is better than merging meaningless tests because we can see the failures in the PR tab.

cmarcelo · 2018-02-05T20:16:29Z

I disagree. The whole point of tests is so that they fail. It is easy to forget about TODOs and skips once something is merged. If that means we merge failing tests now at least we know what is failing and what needs to be fixed. I see no usefulness in tests that should be failing but are passing instead.

If you merge a failing test, the CI output becomes less useful, because every PR and every test execution from now on will be marked as fail. Unless you verify each time: "hey, it's failing because the known failures A, B, C, so its fine", you might let some other failure to creep in. That manual verification is the opposite of what we want to achieve with the CI marking the PR as "checked".

Are those skipped tests useful? They are a better version of the "// TODO: Hey, we should be persisting deletes here.", because they come with the exact code that is expected to be working.

matthewrsj · 2018-02-05T20:20:50Z

If you merge a failing test, the CI output becomes less useful, because every PR and every test execution from now on will be marked as fail. Unless you verify each time: "hey, it's failing because the known failures A, B, C, so its fine", you might let some other failure to creep in. That manual verification is the opposite of what we want to achieve with the CI marking the PR as "checked".

So verify every time. I think for minor tests what you say makes sense. This is a breaking issue though. The updates do not work over several versions until it is fixed. It should be advertised that the bug exists. Painful, but better than relying on memory that the TODO exists.

Are those skipped tests useful? They are a better version of the "// TODO: Hey, we should be persisting deletes here.", because they come with the exact code that is expected to be working.

Sure, but I have an actual test in #134 to test for this

This is all philosophical. I have a fix for the bug that I'll be pushing to a new PR in the next few minutes.

cmarcelo · 2018-02-05T20:21:33Z

This is all philosophical. I have a fix for the bug that I'll be pushing to a new PR in the next few minutes.

Great! So we have a non-problem :-)

cmarcelo · 2018-02-05T20:28:32Z

Updated PR addressing comments.

In the same spirit of testFileSystem struct. It also embeds the testFileSystem, so only one needs to be used in a test. Note that cleanup function will not delete the temporary directory if the test fails, to allow inspection (it also prints the directory name). Two tests were modified to illustrate how it should be used, one that was already using fs and another that wasn't. Signed-off-by: Caio Marcelo de Oliveira Filho <caio.oliveira@intel.com>

Import test from original swupd-server. It checks whether contentsize is adding (and not adding) values correctly. At the moment directories are getting accounted in contentsize (should they), so use an empty bundle to get a baseline. So I checks independent of that factor. Also avoided checking the exact figure for os-core and full, and only do check the size increase between two versions for those. It might make sense to have a separate test that verify the contents of os-core match what is desired, but seemed out of scope of this one. Signed-off-by: Caio Marcelo de Oliveira Filho <caio.oliveira@intel.com>

Import test from original swupd-server. It checks direct includes cause a version bump, and indirect ones don't. This test was already partially present, the new version adds in more checks to cover everything was being checked by the original tests and some more (e.g. validating contents of the packs). Signed-off-by: Caio Marcelo de Oliveira Filho <caio.oliveira@intel.com>

matthewrsj

Would have liked a couple more comments around how the zero pack scanning is done (expects alphabetical order, delta directory should not have any children), but that was optional, so +1.

cmarcelo · 2018-02-05T20:51:15Z

Would have liked a couple more comments around how the zero pack scanning is done (expects alphabetical order, delta directory should not have any children), but that was optional, so +1.

Argh. Read but missed those when doing the second pass.

Import test from original swupd-server. It performs basic checks of a complete execution for a new version: creating fullfiles and a zeropack. Incremented mustValidateZeroPack to check for presence of staged/ and delta/ and their properties. Signed-off-by: Caio Marcelo de Oliveira Filho <caio.oliveira@intel.com>

Update the test so a smaller format is used when creating the next version, and verify that it fails. Signed-off-by: Caio Marcelo de Oliveira Filho <caio.oliveira@intel.com>

Import test from original swupd-server. It checks if deleted files are correctly marked, taking includes into account. I've created some new checks based on swupd-server output. Signed-off-by: Caio Marcelo de Oliveira Filho <caio.oliveira@intel.com>

matthewrsj · 2018-02-05T21:38:06Z

+1 again, thanks for adding the comments.

cmarcelo changed the title ~~Improve test infrastructure and add one more~~ swupd: improve test infrastructure and add one more Feb 2, 2018

cmarcelo added the swupd label Feb 2, 2018

cmarcelo force-pushed the more-tests branch from 52e5b32 to 12fb86e Compare February 3, 2018 07:15

cmarcelo changed the title ~~swupd: improve test infrastructure and add one more~~ swupd: improve test infrastructure and import tests from swupd-server Feb 3, 2018

cmarcelo force-pushed the more-tests branch from a8036d7 to b179f71 Compare February 3, 2018 20:05

This was referenced Feb 3, 2018

Import swupd-server functional tests #138

Closed

Difference in behavior for subtract-delete test compared to swupd-server #139

Closed

matthewrsj reviewed Feb 4, 2018

View reviewed changes

cmarcelo force-pushed the more-tests branch from dc4612a to b1ef7e2 Compare February 5, 2018 20:26

cmarcelo added 3 commits February 5, 2018 12:39

cmarcelo force-pushed the more-tests branch from b1ef7e2 to 3d6724c Compare February 5, 2018 20:40

matthewrsj approved these changes Feb 5, 2018

View reviewed changes

cmarcelo added 3 commits February 5, 2018 13:01

swupd: update TestCreateManifestFormatNoDecrement

dd8055e

Update the test so a smaller format is used when creating the next version, and verify that it fails. Signed-off-by: Caio Marcelo de Oliveira Filho <caio.oliveira@intel.com>

swupd: import subtract-delete test

d34db4b

Import test from original swupd-server. It checks if deleted files are correctly marked, taking includes into account. I've created some new checks based on swupd-server output. Signed-off-by: Caio Marcelo de Oliveira Filho <caio.oliveira@intel.com>

cmarcelo force-pushed the more-tests branch from 3d6724c to d34db4b Compare February 5, 2018 21:01

matthewrsj merged commit a7168b5 into clearlinux:master Feb 5, 2018

cmarcelo deleted the more-tests branch February 5, 2018 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swupd: improve test infrastructure and import tests from swupd-server #136

swupd: improve test infrastructure and import tests from swupd-server #136

cmarcelo commented Feb 2, 2018 •

edited

cmarcelo commented Feb 3, 2018

matthewrsj Feb 4, 2018

cmarcelo Feb 5, 2018

cmarcelo Feb 5, 2018

matthewrsj Feb 5, 2018

matthewrsj commented Feb 4, 2018

matthewrsj commented Feb 4, 2018

matthewrsj Feb 4, 2018

cmarcelo Feb 5, 2018

matthewrsj Feb 4, 2018

cmarcelo Feb 5, 2018

matthewrsj Feb 4, 2018

cmarcelo Feb 5, 2018

matthewrsj commented Feb 4, 2018

matthewrsj commented Feb 4, 2018

matthewrsj Feb 4, 2018

cmarcelo Feb 5, 2018

matthewrsj Feb 4, 2018

matthewrsj Feb 4, 2018

matthewrsj commented Feb 4, 2018

cmarcelo commented Feb 5, 2018

matthewrsj commented Feb 5, 2018

matthewrsj commented Feb 5, 2018

cmarcelo commented Feb 5, 2018

cmarcelo commented Feb 5, 2018

matthewrsj commented Feb 5, 2018 •

edited

cmarcelo commented Feb 5, 2018

matthewrsj commented Feb 5, 2018

cmarcelo commented Feb 5, 2018

cmarcelo commented Feb 5, 2018

matthewrsj left a comment

cmarcelo commented Feb 5, 2018

matthewrsj commented Feb 5, 2018

swupd: improve test infrastructure and import tests from swupd-server #136

swupd: improve test infrastructure and import tests from swupd-server #136

Conversation

cmarcelo commented Feb 2, 2018 • edited

cmarcelo commented Feb 3, 2018

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

matthewrsj commented Feb 4, 2018

matthewrsj commented Feb 4, 2018

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

matthewrsj commented Feb 4, 2018

matthewrsj commented Feb 4, 2018

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

matthewrsj commented Feb 4, 2018

cmarcelo commented Feb 5, 2018

matthewrsj commented Feb 5, 2018

matthewrsj commented Feb 5, 2018

cmarcelo commented Feb 5, 2018

cmarcelo commented Feb 5, 2018

matthewrsj commented Feb 5, 2018 • edited

cmarcelo commented Feb 5, 2018

matthewrsj commented Feb 5, 2018

cmarcelo commented Feb 5, 2018

cmarcelo commented Feb 5, 2018

matthewrsj left a comment

Choose a reason for hiding this comment

cmarcelo commented Feb 5, 2018

matthewrsj commented Feb 5, 2018

cmarcelo commented Feb 2, 2018 •

edited

matthewrsj commented Feb 5, 2018 •

edited